[GLUTEN-12597][CORE] Migrate ReadRel text read options to Substrait 0.98 (TextReadOptions -> DelimiterSeparatedTextReadOptions) - #12917
Draft
nielspardon wants to merge 1 commit into
Conversation
….98 (TextReadOptions -> DelimiterSeparatedTextReadOptions) Part of apache#12597.
|
Run Gluten Clickhouse CI on x86 |
25 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Part of #12597.
Rebases the text read options in
ReadRel.LocalFiles.FileOrFilesonto Substraitv0.98.0, where the fork'sTextReadOptionsbecameDelimiterSeparatedTextReadOptions. Upstream's six fields are adopted verbatim (field_delimiter1,max_line_size2,quote3,header_lines_to_skip4,escape5,optional string value_treated_as_null6).header_lines_to_skipandvalue_treated_as_nullare the fork'sheader(was 5) andnull_value(was 7) renamed with the same meaning, and the deprecatedschemafield is dropped (nothing reads or writes it — thesetSchemacall inLocalFilesNodetargets the file-levelFileOrFiles.schema, which is unrelated). Gluten's two genuinely-local knobs move to the 1000 graft range documented inSubstraitModifications.md:max_block_sizeto 1000 andempty_as_defaultto 1001. Thefile_formatoneof keepstextat tag 14, which is also upstream's number.The one subtlety this PR is careful about: the fork put
max_block_size(rows per output block, a ClickHouse batching knob) on tag 2 — exactly where 0.98 putsmax_line_size(max bytes per line, a parse limit). These are different concepts with the same wire type, so conflating them by renaming the field would compile and silently change meaning. Insteadmax_line_sizeis vendored verbatim at tag 2 (Gluten produces no value for it) andmax_block_sizeis relocated to the 1000 graft, keeping the two as distinct fields.Because tags 2 and 6 are reused with compatible wire types but different meanings, a JAR and a native library built from opposite sides of this change mis-read each other silently rather than failing — in particular old tag 6 (
escape, which Gluten's Hive text path always sets) now parses asvalue_treated_as_null. The JAR and native library must therefore be rebuilt together; this is called out in the proto comment and inSubstraitModifications.md.value_treated_as_nullisoptionalin 0.98, so setting it — even to the empty string — declares "this value is null and the file is entirely nullable strings". The producer therefore leaves it unset when the reader supplied nonullValue, and the ClickHouse consumer keys off field presence (has_value_treated_as_null()) rather than emptiness. This preserves Gluten's existing "no nullValue supplied" behavior.JsonReadOptionskeeps its Gluten-local fork until upstream adds equivalent JSON read options.How was this patch tested?
Standalone
protoccompile of the proto (well-formed, no duplicate tags). Local JVM build and the newDelimiterSeparatedTextReadOptionsProtoSuite, which drives the realLocalFilesNodeproducer and pins the descriptor: the six upstream tags,value_treated_as_null's presence tracking, themax_block_size(1000) vsmax_line_size(2) split, the dropped/renamed field names, thetextoneof entry, and the optional-presence contract onvalue_treated_as_null. The ClickHouse consumer (ExcelTextFormatFile.cpp) is exercised only in ClickHouse CI (native build needs a Linux/Docker toolchain), so its edits were verified by inspection; thegtest_parser.cppfixture was updated to the renamed proto-JSON field name. The Velox side has no text-options code path.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)
🤖 Generated with AI