Skip to content

Fix bad parameter count in code generator#7014

Merged
dain merged 1 commit intotrinodb:masterfrom
dain:block-position-args
Feb 27, 2021
Merged

Fix bad parameter count in code generator#7014
dain merged 1 commit intotrinodb:masterfrom
dain:block-position-args

Conversation

@dain
Copy link
Copy Markdown
Member

@dain dain commented Feb 24, 2021

Avoid BLOCK_POSITION when there a lot of function arguments

@cla-bot cla-bot bot added the cla-signed label Feb 24, 2021
Copy link
Copy Markdown
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

Please add a unit test.
Also, please add an integration test ensuring there isn't a similar limitation elsewhere.

@wendigo
Copy link
Copy Markdown
Contributor

wendigo commented Feb 24, 2021

One of the tests is failing right now:

2021-02-24T04:48:55.6686772Z [ERROR] testVersionOnCompilerFailedError(io.trino.tests.TestServer)  Time elapsed: 0.878 s  <<< FAILURE!
2021-02-24T04:48:55.6690974Z java.lang.RuntimeException: Error expected
2021-02-24T04:48:55.6696818Z 	at io.trino.tests.TestServer.lambda$checkVersionOnError$16(TestServer.java:289)
2021-02-24T04:48:55.6701969Z 	at java.base/java.util.Optional.orElseThrow(Optional.java:408)
2021-02-24T04:48:55.6707765Z 	at io.trino.tests.TestServer.checkVersionOnError(TestServer.java:289)
2021-02-24T04:48:55.6714478Z 	at io.trino.tests.TestServer.testVersionOnCompilerFailedError(TestServer.java:278)
2021-02-24T04:48:55.6721289Z 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2021-02-24T04:48:55.6729674Z 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
2021-02-24T04:48:55.6738202Z 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2021-02-24T04:48:55.6748072Z 	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
2021-02-24T04:48:55.6759858Z 	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
2021-02-24T04:48:55.6767042Z 	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
2021-02-24T04:48:55.6775082Z 	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
2021-02-24T04:48:55.6781653Z 	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
2021-02-24T04:48:55.6789872Z 	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
2021-02-24T04:48:55.6796166Z 	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
2021-02-24T04:48:55.6803433Z 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
2021-02-24T04:48:55.6810765Z 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
2021-02-24T04:48:55.6815305Z 	at java.base/java.lang.Thread.run(Thread.java:834)

@dain
Copy link
Copy Markdown
Member Author

dain commented Feb 24, 2021

@findepi I fixed the test failure... it was forcing a compiler failure using the same pivot technique. I changed it to another query that also fails during compilation. @wendigo has a PR with an additional test for this case.

@dain dain requested a review from findepi February 24, 2021 23:13
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems redundant, this is what test method name is saying

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I like comments like this as it helps me when updating to know if the exact query shape was important.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the query can select from VALUES and thus you don't need to DROP and CREATE here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Using values is dangerous as it can be optimized away. I'm just going to leave the query as is.

Avoid BLOCK_POSITION when there a lot of function arguments
@dain dain force-pushed the block-position-args branch from a95787f to c4ac0dc Compare February 27, 2021 04:21
@dain dain merged commit f45869f into trinodb:master Feb 27, 2021
@dain dain deleted the block-position-args branch February 27, 2021 15:56
@dain dain mentioned this pull request Mar 2, 2021
10 tasks
@martint martint added this to the 353 milestone Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants