Analyze table function arguments #4

[mohsaka]

Changes adapted from trino/PR#13602

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

• Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
• PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
• Documented new properties (with its default value), SQL syntax, functions, or other functionality.
• If release notes are required, they follow the release notes guidelines.
• Adequate tests were added if applicable.
• CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... 
* ... 

Hive Connector Changes
* ... 
* ... 

If release note is NOT required, use:

== NO RELEASE NOTE ==

[mohsaka]

Completed up to 31b2b79751944eed42eaa4538a52dbf65c0399a6

[mohsaka]

Latest update, almost finished the last commit. Needs to bring in the Statement Analyzer into AbstractAnalyzer test. Afterwards we can work test by test to fix the issues that occur.

[xin-zhang2]

Lastest update:
Adde the table functions in AbstractAnalyzerTest.
Fixed some failures in the tests.

The current failure is in testDescriptorArgument

java.lang.AssertionError: Expected error 'line 1:80: Unknown type: verybigint', but got 'verybigint'

	at org.testng.Assert.fail(Assert.java:98)
	at com.facebook.presto.sql.analyzer.AbstractAnalyzerTest.assertFails(AbstractAnalyzerTest.java:473)
	at com.facebook.presto.sql.analyzer.AbstractAnalyzerTest.assertFails(AbstractAnalyzerTest.java:425)
	at com.facebook.presto.sql.analyzer.TestAnalyzer.testDescriptorArgument(TestAnalyzer.java:2076)

Caused by: com.facebook.presto.sql.analyzer.SemanticException: verybigint
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.lambda$null$30(StatementAnalyzer.java:1594)
	at java.util.Optional.map(Optional.java:215)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.lambda$null$31(StatementAnalyzer.java:1589)
	...

It is caused by a signature mismatch of SemanticException, and the code lies in

presto/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

Lines 1589 to 1595 in 699b0ad

	field.getType().map(type -> {
	try {
	return functionAndTypeResolver.getType(parseTypeSignature(type));
	}
	catch (IllegalArgumentException e) {
	throw new SemanticException(TYPE_MISMATCH, type, "Unknown type: %s", type);
	}

where type is a String in Presto but a DataType which extends Node in Trino.

The same issue also occurs intestCopartitioning

presto/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

Lines 1698 to 1702 in 699b0ad

	if (!referencedArguments.add(argument.getArgumentName())) {
	// multiple references to argument in COPARTITION clause are implicitly prohibited by
	// ISO/IEC TR REPORT 19075-7, p.33, Feature B203, “More than one copartition specification”
	throw new SemanticException(INVALID_COPARTITIONING, name.getOriginalParts().get(0), "Multiple references to table argument: %s in COPARTITION clause", name);
	}

where originalParts is a list of String in Presto but a list of Identifier in Trino.

In order to indicate the accurate location in the error message, I think we should choose the trino approach and refactor the type field in DescriptorField and originalParts in QualifiedName
@mohsaka What's your thought on this?

[xin-zhang2]

@mohsaka
The following message contains special characters in regular expression that results in a failure in the matches, so I temporarily added the equals comparison.

presto/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

Line 1457 in 699b0ad

throw new SemanticException(INVALID_FUNCTION_ARGUMENT, argument, "Invalid descriptor argument %s. Descriptors should be formatted as 'DESCRIPTOR(name [type], ...)'", (Object) argumentSpecification.getName());

[mohsaka]

Lastest update: Adde the table functions in AbstractAnalyzerTest. Fixed some failures in the tests.

The current failure is in testDescriptorArgument

java.lang.AssertionError: Expected error 'line 1:80: Unknown type: verybigint', but got 'verybigint'

	at org.testng.Assert.fail(Assert.java:98)
	at com.facebook.presto.sql.analyzer.AbstractAnalyzerTest.assertFails(AbstractAnalyzerTest.java:473)
	at com.facebook.presto.sql.analyzer.AbstractAnalyzerTest.assertFails(AbstractAnalyzerTest.java:425)
	at com.facebook.presto.sql.analyzer.TestAnalyzer.testDescriptorArgument(TestAnalyzer.java:2076)

Caused by: com.facebook.presto.sql.analyzer.SemanticException: verybigint
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.lambda$null$30(StatementAnalyzer.java:1594)
	at java.util.Optional.map(Optional.java:215)
	at com.facebook.presto.sql.analyzer.StatementAnalyzer$Visitor.lambda$null$31(StatementAnalyzer.java:1589)
	...

It is caused by a signature mismatch of SemanticException, and the code lies in

presto/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

Lines 1589 to 1595 in 699b0ad

	field.getType().map(type -> {
	try {
	return functionAndTypeResolver.getType(parseTypeSignature(type));
	}
	catch (IllegalArgumentException e) {
	throw new SemanticException(TYPE_MISMATCH, type, "Unknown type: %s", type);
	}

where type is a String in Presto but a DataType which extends Node in Trino.

The same issue also occurs intestCopartitioning

presto/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

Lines 1698 to 1702 in 699b0ad

	if (!referencedArguments.add(argument.getArgumentName())) {
	// multiple references to argument in COPARTITION clause are implicitly prohibited by
	// ISO/IEC TR REPORT 19075-7, p.33, Feature B203, “More than one copartition specification”
	throw new SemanticException(INVALID_COPARTITIONING, name.getOriginalParts().get(0), "Multiple references to table argument: %s in COPARTITION clause", name);
	}

where originalParts is a list of String in Presto but a list of Identifier in Trino.
In order to indicate the accurate location in the error message, I think we should choose the trino approach and refactor the type field in DescriptorField and originalParts in QualifiedName @mohsaka What's your thought on this?

For this one, we should just replace the first type with field since in Presto all of our types are stored as String instead of a DataType object.

                                                        catch (IllegalArgumentException e) {
                                                            throw new SemanticException(TYPE_MISMATCH, field, "Unknown type: %s", type);
                                                        }

Made that change as well as a location change, and it looks good now. Great work!

We did think about refactoring to DataType object but we thought it would be too invasive for this project Hopefully we can avoid making this change.

[mohsaka]

Update, started 13653 on the same PR since it is small. Will add 14115 to this PR as well since it is also small.

[xin-zhang2]

Finished 13653 and 14115.

I made the changes to use MISSING_ATTRIBUTE instead of COLUMN_NOT_FOUND in the test cases, as the SemanticErrorCode for missingAttributeException differ between Presto and Trino.

Also comment out some tests in testTableFunctionInvocationContext as MATCH_RECOGNIZE(trinodb/trino#6550) is not supported in Presto.