-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
[FLINK-26712][table-planner] Metadata keys should not conflict with physical columns #19236
Conversation
@@ -99,6 +99,25 @@ class TableScanTest extends TableTestBase { | |||
util.verifyExecPlan("SELECT * FROM MetadataTable") | |||
} | |||
|
|||
@Test | |||
def testDDLWithMetadataThatConflictsWithPhysicalColumn(): Unit = { |
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.
Should the same test be done for the sink table as well?
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.
+1
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.
Looks good @twalthr . Left a few comments.
@@ -71,6 +71,9 @@ | |||
@Internal | |||
public final class DynamicSourceUtils { | |||
|
|||
// Ensures that physical and metadata columns don't collide. | |||
public static final String METADATA_COLUMN_PREFIX = "$metadata$"; |
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.
minor, but maybe we can define this constant in a common place for both source and sink?
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.
the source and sink stack are completely separate, that's why we also have separate util classes
IntStream.range(0, metadata.size()) | ||
.mapToObj( | ||
i -> { | ||
// Access metadata columns from behind because the |
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.
from behind -> backwards
, or in reverse
?
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 looked it up and chose from the back
@@ -99,6 +99,25 @@ class TableScanTest extends TableTestBase { | |||
util.verifyExecPlan("SELECT * FROM MetadataTable") | |||
} | |||
|
|||
@Test | |||
def testDDLWithMetadataThatConflictsWithPhysicalColumn(): Unit = { |
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.
+1
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.
LGTM Thank you @twalthr !
…hysical columns This reduces the likelihood for name collisions between metadata columns and physical columns. It might break some connector implementations that used SupportsReadable/WritingMetadata and name-based column arithmetics. We don't recommend using name-based column arithmetics but index-based ones. This closes apache#19236.
…hysical columns This reduces the likelihood for name collisions between metadata columns and physical columns. It might break some connector implementations that used SupportsReadable/WritingMetadata and name-based column arithmetics. We don't recommend using name-based column arithmetics but index-based ones. This closes #19236.
…hysical columns This reduces the likelihood for name collisions between metadata columns and physical columns. It might break some connector implementations that used SupportsReadable/WritingMetadata and name-based column arithmetics. We don't recommend using name-based column arithmetics but index-based ones. This closes apache#19236.
…hysical columns This reduces the likelihood for name collisions between metadata columns and physical columns. It might break some connector implementations that used SupportsReadable/WritingMetadata and name-based column arithmetics. We don't recommend using name-based column arithmetics but index-based ones. This closes apache#19236.
…hysical columns This reduces the likelihood for name collisions between metadata columns and physical columns. It might break some connector implementations that used SupportsReadable/WritingMetadata and name-based column arithmetics. We don't recommend using name-based column arithmetics but index-based ones. This closes apache#19236.
…hysical columns This reduces the likelihood for name collisions between metadata columns and physical columns. It might break some connector implementations that used SupportsReadable/WritingMetadata and name-based column arithmetics. We don't recommend using name-based column arithmetics but index-based ones. This closes apache#19236.
…hysical columns This reduces the likelihood for name collisions between metadata columns and physical columns. It might break some connector implementations that used SupportsReadable/WritingMetadata and name-based column arithmetics. We don't recommend using name-based column arithmetics but index-based ones. This closes apache#19236.
…hysical columns This reduces the likelihood for name collisions between metadata columns and physical columns. It might break some connector implementations that used SupportsReadable/WritingMetadata and name-based column arithmetics. We don't recommend using name-based column arithmetics but index-based ones. This closes apache#19236.
…hysical columns This reduces the likelihood for name collisions between metadata columns and physical columns. It might break some connector implementations that used SupportsReadable/WritingMetadata and name-based column arithmetics. We don't recommend using name-based column arithmetics but index-based ones. This closes apache#19236.
What is the purpose of the change
This reduces the likelihood for name collisions between metadata columns and physical columns. It might break some connector implementations that used
SupportsReadable/WritingMetadata
and name-based column arithmetics. Since this might only be done by very advanced connector implementations (such as our FileSystem connector) and we don't recommend using name-based column arithmetics but index-based ones. This change should not affeect many users and should be done asap as it changes the JSON plan.Brief change log
$metadata$
Verifying this change
This change added tests and can be verified as follows:
TableScanTest
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: yesDocumentation