Flink: Fix read split table properties being ignored - #17445
Conversation
| conf = new FlinkReadConf(table, ImmutableMap.of(), new Configuration()); | ||
| assertThat(conf.splitFileOpenCost()).isEqualTo(TableProperties.SPLIT_OPEN_FILE_COST_DEFAULT); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new test exercises only three of the four precedence tiers (flinkConf > tableProperty > default) for each split option; the top tier; an explicit entry in the readOptions map (the second FlinkReadConf constructor argument) is never asserted. All three tests pass ImmutableMap.of() as the readOptions arg, so the test suite would pass even if the read-option tier were completely broken. Add a fourth assertion per method: instantiate with e.g. ImmutableMap.of(FlinkReadOptions.SPLIT_SIZE, "999") and assert the result beats both the flinkConf value and the table property.
|
Thanks for the review @uros-b .Added assertions for the read options tier in all three tests. |
mxm
left a comment
mxm
left a comment
There was a problem hiding this comment.
Thanks for the PR @vishnuprakaz!
|
Thanks for the review @mxm! |
|
@vishnuprakaz Could you open a new PR with a backport for Flink versions 2.0 and 1.20? |
|
Thanks @mxm! Sure, will do a backport for Flink 2.0 and 1.20. |
4 participants
Setting
read.split.target-size,read.split.planning-lookback, orread.split.open-file-costas a table property has no effect on Flink reads. The split options inFlinkReadOptionsdeclare default values, soFlinkConfParseralways returns the Flink default and never reaches the table property fallback inFlinkReadConf, even though the docs promise read option, then Flink configuration, then table property. This is a regression from #5967, which introduced the read options with declared defaults; before that the planner only set the split scan options when explicitly configured and the table properties were honored.The split options now declare no default, like the write options, and the defaults are applied in
FlinkReadConfafter the table property lookup. Needs a backport to 1.20 and 2.0