[FLINK-39286][table] Introduce immutable columns constraint in schema#27809
[FLINK-39286][table] Introduce immutable columns constraint in schema#27809xuyangzhong wants to merge 7 commits intoapache:masterfrom
Conversation
410ba27 to
aefd4bf
Compare
|
@xuyangzhong Can you rebase the master? There was a known issue that just fixed (#27808). |
aefd4bf to
6e71040
Compare
lincoln-lil
left a comment
There was a problem hiding this comment.
@xuyangzhong Thanks for the pr! Overall looks good. I left some comments there.
One question regarding the SHOW CREATE TABLE path: I understand this FLIP does not yet expose the constraint via DDL syntax, but if we silently omit the new constraint from the output, it could break DDL round-trip fidelity, WDYT?
...r/src/main/java/org/apache/flink/table/planner/plan/metadata/FlinkRelMdImmutableColumns.java
Outdated
Show resolved
Hide resolved
...nk-table-common/src/main/java/org/apache/flink/table/catalog/ImmutableColumnsConstraint.java
Outdated
Show resolved
Hide resolved
...va/org/apache/flink/table/planner/plan/nodes/exec/serde/ImmutableColumnsConstraintMixin.java
Outdated
Show resolved
Hide resolved
| import java.util.stream.Collectors; | ||
|
|
||
| /** | ||
| * A constraint for immutable columns. All columns within each pk in this constraint will not be |
There was a problem hiding this comment.
The description here may be misleading regarding its relationship with the PK. The JavaDoc for Schema.Builder.immutableColumns is clearer.
Currently, |
6e71040 to
48f2018
Compare
|
Hi, @lincoln-lil I have addressed all comments and added a new commit about https://issues.apache.org/jira/browse/FLINK-39314 in this pr. Can you please take a look again? |
I checked these changes, looks semantically correct and addresses a real non-opt issue where only the first upsert key was checked instead of all available upsert keys. The existing tests provide partial coverage, so additional tests for the multi-upsertkey scenario would be beneficial(not strictly required). |
…implement the derivation logic of some simple nodes and update the logic to infer upsert key with immutable columns
…ter is applied on any of upsert keys
c0a0197 to
0774f24
Compare
|
Thanks for reminder. I have added more tests for the commit |
What is the purpose of the change
This pr contains 2 parts:
Brief change log
Verifying this change
New tests are added to verify this pr.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): yesDocumentation