flink icon indicating copy to clipboard operation
flink copied to clipboard

[FLINK-35827][table-planner] Correcting equality comparisons between rowType fields and constants

Open xuyangzhong opened this issue 1 year ago • 2 comments

What is the purpose of the change

The first commit tries to remove the redundant cast in row type with PEEK_FIELDS_NO_EXPAND. The second commit tries to fix the wrong equality comparison result between rowType fields and constants.

Brief change log

  • When trying to find the least restrictive StructuredType between row types with PEEK_FIELDS_NO_EXPAND, it should not return row types with FULLY_QUALIFIED.
  • The classes with two RowData may be different. When we check the equality of row types, we should check the type of RowData first, just like checking array, map and other types.
  • Add tests to verify these two commits.

Verifying this change

Tests are added to verify these two commits.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented?

xuyangzhong avatar Aug 21 '24 09:08 xuyangzhong

CI report:

  • eff463efd44432a0daaa5c2542e571cc9ca966c4 Azure: SUCCESS
Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

flinkbot avatar Aug 21 '24 09:08 flinkbot

If commit-1 is missing, the bug fixed by commit-2 will only appear in the Table API and not in the SQL API. This is because the SQL API will always cast the PEEK_FIELDS_NO_EXPAND RowType to FULLY_QUALIFIED by Calcite, resulting in no issues since every RowType data will be in BinaryRowData. However, it’s important to note that in Flink, the StructKind of RowType should consistently be the PEEK_FIELDS_NO_EXPAND throughout the entire process.

xuyangzhong avatar Aug 21 '24 09:08 xuyangzhong

If commit-1 is missing, the bug fixed by commit-2 will only appear in the Table API and not in the SQL API. This is because the SQL API will always cast the PEEK_FIELDS_NO_EXPAND RowType to FULLY_QUALIFIED by Calcite, resulting in no issues since every RowType data will be in BinaryRowData. However, it’s important to note that in Flink, the StructKind of RowType should consistently be the PEEK_FIELDS_NO_EXPAND throughout the entire process.

@xuyangzhong thanks for fixing this! The 1st commit seems not a necessary change for this fix, if there exists a concrete case in sql area, we can fix it separately, WDYT?

lincoln-lil avatar Sep 03 '24 06:09 lincoln-lil

@lincoln-lil Agree with you. I have updated this pr and removed the first commit.

xuyangzhong avatar Sep 05 '24 06:09 xuyangzhong