[FLINK-39015][table] Fix key extractor for multi join. Change GenericRowData to BinaryRowData

[ldadima]

What is the purpose of the change

To fix FLINK-39015

Verifying this change

Run JoinITCase#testInnerMultiJoinWithEqualPk

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? (not applicable)

[flinkbot]

CI report:

• dbfc0da Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run azure re-run the last Azure build

[ldadima]

Hey @snuyanzin Could you review, please?

[gustavodemorais]

Looks great! Thanks for picking this up @ldadima.

Could you just move the test as I mentioned in the comment? Thanks

[gustavodemorais]

I think it either makes sense to

1. Have the whole suite running with this the MULTI_JOIN flag as a param
2. Add this to MultiJoinSemanticTests

Since 1. is high LOE, for this PR, I'd just go with 2. for now.

[ldadima]

Thanks for your review @gustavodemorais

1 Checked. All passed

2 I don't understand why we need to add SemanticTest. The problem only occurs after restoring from a checkpoint and only for HeapStateBackend. Also in SemanticTest there is MULTI_JOIN_TWO_WAY_INNER_JOIN_WITH_WHERE_IN, which checks similar sql (uses the same MultiJoinStateViews.JoinKeyContainsUniqueKey). I suggest to modify MultiJoinITCase (here) for failingDataSource and add this case

[gustavodemorais]

Hey @ldadima, if it only happens after restoring from a checkpoint, then it'd make sense to move it to `MultiJoinRestoreTest?

[ldadima]

Hi @gustavodemorais, You're right, but this 'MultiJoinRestoreTest uses RocksdbStateBackend (here), and to reproduce the issue, HeapStateBackend is needed.
I think MultiJoinITCase is more convinient. Also possible to add heap case for MultiJoinRestoreTest
What do you think?

[gustavodemorais]

• I think we can merge it like this and move it to MultiJoinITCase here 🙂
• Restore tests validate plan compatibility, not state backend behavior so I think it we don't want to do that for now

[ldadima]

Great. Thanks for your help

[gustavodemorais]

Thanks for the fix and contribution @ldadima. LGTM

[gustavodemorais]

Hey @ldadima, we're discussing some possible improvements around the row equality check before we merge this and I'll get back to you.