Skip to content

Fix and test table functions#12951

Merged
findepi merged 3 commits intotrinodb:masterfrom
kasiafi:370FixNoArguments
Jul 2, 2022
Merged

Fix and test table functions#12951
findepi merged 3 commits intotrinodb:masterfrom
kasiafi:370FixNoArguments

Conversation

@kasiafi
Copy link
Copy Markdown
Member

@kasiafi kasiafi commented Jun 23, 2022 •
edited
Loading

Release notes

# General
* Fix query failure when no arguments are passed to a table function.

// here I'm not sure about the category. The bug was in SPI, but there is no SPI change
* Fix potential failure when declaring default arguments to a table function.

No docs needed.

@cla-bot cla-bot bot added the cla-signed label Jun 23, 2022
findepi
findepi approved these changes Jun 23, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need !arguments.isEmpty() &&?

Copy link
Copy Markdown
Member Author

@kasiafi kasiafi Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the purpose of analysis, we need to choose if the case of no passed arguments falls under "arguments passed by name" or "arguments passed positionally". I chose the latter.

Copy link
Copy Markdown
Member

@findepi findepi Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would maybe be more symmetric if you had variables

anyArgumentPassedByName
anyArgumentPassedByPosition

@kasiafi kasiafi force-pushed the 370FixNoArguments branch 2 times, most recently from 400cd64 to 2eeb588 Compare June 29, 2022 12:49
@kasiafi
Copy link
Copy Markdown
Member Author

kasiafi commented Jun 29, 2022

@findepi I added tests. Also, I fixed another bug. PTAL

@kasiafi kasiafi changed the title Fix failure when no arguments are passed to a table function. Fix and test table functions. Jun 29, 2022
@kasiafi kasiafi changed the title Fix and test table functions. Fix and test table functions Jun 29, 2022
findepi
findepi approved these changes Jun 29, 2022
View reviewed changes
testing/trino-tests/src/test/java/io/trino/tests/TestTableFunctionInvocation.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

io.trino.testing.DistributedQueryRunner#builder ?

you don't seem to need tpch here.

core/trino-spi/src/main/java/io/trino/spi/ptf/ScalarArgumentSpecification.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see

trino/testing/trino-testing/src/main/java/io/trino/testing/tpch/AppendingRecordSet.java

Line 46 in 03d8e5a

checkArgument(Primitives.wrap(appendedTypes.get(i).getJavaType()).isInstance(value), "Object value does not match declared type");

for how to avoid special casing for primitive types

testing/trino-tests/src/test/java/io/trino/tests/TestTableFunctionInvocation.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slightly related: #13032

assertQuery doesn't verify schema of the result at all, so please remove the comment.

@kasiafi kasiafi force-pushed the 370FixNoArguments branch from 2eeb588 to 460ff4f Compare June 30, 2022 12:29
findepi
findepi approved these changes Jun 30, 2022
View reviewed changes
testing/trino-tests/src/test/java/io/trino/tests/TestTableFunctionInvocation.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant, remove.

testing/trino-tests/src/test/java/io/trino/tests/TestTableFunctionInvocation.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestPolymorphicTableFunction ?

Copy link
Copy Markdown
Member Author

@kasiafi kasiafi Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is meant for testing table functions in general, not only the polymorphic ones. I also want to mention "invocation" in the class name, as we might likely add tests for declaring table function.

testing/trino-tests/src/test/java/io/trino/tests/TestTableFunctionInvocation.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bigint -> Primitive

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would maybe be more symmetric if you had variables

anyArgumentPassedByName
anyArgumentPassedByPosition

@kasiafi
Copy link
Copy Markdown
Member Author

kasiafi commented Jul 1, 2022

it would maybe be more symmetric if you had variables

It would be asymmetric elsewhere, because I would still need to handle the case of no arguments.

kasiafi added 3 commits July 1, 2022 11:02
@kasiafi
Add table functions to mock connector
b602e4e
@kasiafi
Fix failure when declaring default arguments for a table function
98fc1ee 
The failure occured when the argument type had a primitive
java type. Due to boxing, the type check failed.
@kasiafi
Fix failure when no arguments are passed to a table function
0cec709 
The failure occured when the table function had a non-zero
number of arguments, all of the arguments were declared as not
required (default values), and the function was invoked with
no arguments.
@kasiafi kasiafi force-pushed the 370FixNoArguments branch from 460ff4f to 0cec709 Compare July 1, 2022 09:03
@findepi findepi merged commit 31de900 into trinodb:master Jul 2, 2022
@github-actions github-actions bot added this to the 389 milestone Jul 2, 2022
@colebow colebow mentioned this pull request Jul 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findepi findepi findepi approved these changes

Assignees

No one assigned

Labels

cla-signed

Milestone

389

Development

Successfully merging this pull request may close these issues.

2 participants

@kasiafi @findepi