Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix dereference in SQL routine #20181

Merged
merged 1 commit into from
Dec 21, 2023
Merged

Fix dereference in SQL routine #20181

merged 1 commit into from
Dec 21, 2023

Conversation

findepi
Copy link
Member

@findepi findepi commented Dec 20, 2023

Fixes #19997

@cla-bot cla-bot bot added the cla-signed label Dec 20, 2023
@findepi findepi mentioned this pull request Dec 20, 2023
Closed
findepi
findepi commented Dec 20, 2023
View reviewed changes
testing/trino-tests/src/test/java/io/trino/tests/TestLocalEngineOnlyQueries.java Outdated
Comment on lines 68 to 76
// TODO Fix LocalQueryRunner execution of functions
assertThatThrownBy(super::testIssue19997)
.isInstanceOf(AssertionError.class)
.hasMessageContaining("[Rows for query [WITH FUNCTION format_topn(input map<varchar, bigint>)")
.hasMessageContaining("""
Expecting actual:
(null, null, null)
to contain exactly in any order:
[([AAA, AAA, BBB, BBB, BBB, CCC, CCC, CCC, DDD, DDD], {AAA=2, CCC=3, BBB=3}, CCC=3, BBB=3, AAA=2)]""");
Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently LocalQueryRunner and DistributedQueryRunner return different results for this test.

cc @electrum

Copy link
Member Author

Choose a reason for hiding this comment

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

reported as #20190

@findepi findepi force-pushed the findepi/fix-dereference-in-sql-routine-2f9db1 branch 3 times, most recently from 40acf4b to f91b601 Compare December 20, 2023 21:21
@findepi findepi force-pushed the findepi/fix-dereference-in-sql-routine-2f9db1 branch from f91b601 to 873410b Compare December 21, 2023 09:34
@findepi
Fix ConstantExpression serialization
e567dbf 
When `ConstantExpression` was created with a Trino type and a value
that didn't match this type's java type (for example Trino INTEGER with
value being Integer, instead of Long), the `ConstantExpression` would
fail to serialize to JSON. Such failure happens during sending task
status updates to workers and is currently logged and ignored, leading
to query hang. The problem could be triggered with SQL routines, which
utilize `RowExpression` (including `ConstantExpression`) serialization.

Thus, this fixes execution of SQL routines involving Row field
dereference.
@findepi findepi force-pushed the findepi/fix-dereference-in-sql-routine-2f9db1 branch from 873410b to e567dbf Compare December 21, 2023 09:42
@findepi
Copy link
Member Author

findepi commented Dec 21, 2023

AC

@findepi
Copy link
Member Author

findepi commented Dec 21, 2023

CI #20199 + artifact-checks fail (As freq)

thanks @wendigo for restarting

@wendigo wendigo merged commit 05404da into trinodb:master Dec 21, 2023
88 checks passed
@github-actions github-actions bot added this to the 436 milestone Dec 21, 2023
@findepi findepi deleted the findepi/fix-dereference-in-sql-routine-2f9db1 branch December 21, 2023 13:05
@mosabua
Copy link
Member

mosabua commented Dec 21, 2023

Nice!

@colebow colebow mentioned this pull request Jan 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nineinchnick nineinchnick nineinchnick left review comments

@martint martint martint left review comments

@wendigo wendigo wendigo approved these changes

@losipiuk losipiuk Awaiting requested review from losipiuk

Assignees
No one assigned
Labels
cla-signed
Milestone
436
Development

Successfully merging this pull request may close these issues.

SQL routine dereferencing a Row field hangs indefinitely
5 participants
@findepi @mosabua @martint @wendigo @nineinchnick