-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
Add catalogVersion
into QueryInputMetadata
& QueryOutputMetadata
#17497
Add catalogVersion
into QueryInputMetadata
& QueryOutputMetadata
#17497
Conversation
cb7422e
to
83c74d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this looks good, but would it be possible to use CatalogVersion
instead of String
in the Input/Output analysis parts. It's fine to keep it as String in the eventlistener parts.
@@ -32,6 +32,7 @@ | |||
public final class Input | |||
{ | |||
private final String catalogName; | |||
private final String catalogVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anyway we can use the Object form of CatalogVersion
here instead of string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the type of the field to Object
instead of String
/ CatalogHandle.CatalogVersion
.
The only constraint is now on the constructor to have CatalogHandle.CatalogVersion
as a parameter so that it knows during the deserialization that the value
"catalogVersion": "default"
is to be converted back to a CatalogHandle.CatalogVersion
instead of String
like it would normally happen without this extra information.
83c74d5
to
96a74fc
Compare
We could add a test on trino/core/trino-main/src/main/java/io/trino/connector/FileCatalogStore.java Lines 170 to 186 in ebed7dc
The current hashed value of the input is |
96a74fc
to
bdadd6f
Compare
core/trino-main/src/main/java/io/trino/sql/analyzer/Analysis.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/Output.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/Output.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/sql/analyzer/TestOutput.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/execution/TestQueryStateMachine.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/eventlistener/QueryInputMetadata.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/eventlistener/QueryOutputMetadata.java
Outdated
Show resolved
Hide resolved
@findinpath, sorry I think I probably wasn't clear enough last time. What I meant was whether we could use the proper |
0adf834
to
5f75319
Compare
5f75319
to
ada035f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the tests pass, this looks good to me.
@electrum can you get this merged when ready?
@electrum ping |
Description
As a follow-up on @dain 's work for adding
version
to theCatalogHandle
, this PR addscatalogVersion
to theinputs
andoutput
fields fromQueryIOMetadata
to eventually distinguish the catalog version from the output sent to the event listener for the tables involved in Trino queries.Additional context and related issues
Release notes
(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: