Repository navigation
Conversation
### What problem does this PR solve? Issue Number: close apache#68890 Related PR: apache#68883 Problem Summary: Stream Load with `format=arrow` could not load the Arrow forms clients send for a VARIANT column. `DataTypeVariantV2SerDe::read_column_from_arrow` accepted only the exact shape Doris writes for Arrow Flight, `struct<metadata: binary, value: binary>` in that field order. It rejected the other shapes: - JSON text, one document per row (`utf8`, `large_utf8`, `binary`, `large_binary`), which is how most clients send semi-structured events; - `large_binary` children, or the two fields in the other order (pyarrow builds the struct by name); - a dictionary-encoded `metadata` child. A producer that encodes a record batch at a time has one metadata blob per batch. As a plain binary child, that blob is repeated in every row: about 1 KB per row for a batch mixing ~10 event sources and about 45 KB per row for one mixing all of them, against ~1.3 KB of value bytes. As `dictionary<int32, binary>` with one entry, it costs a few bytes per row. On the reader side, FileScannerV2's Arrow stream reader normalizes every column before the SerDe sees it, which decodes a dictionary into plain binary. For dictionary metadata that copies the shared blob into every row, and the SerDe then compares it row by row to find that all rows share it. This PR: 1. Extends the VARIANT V2 Arrow reader to accept, with or without the `arrow.parquet.variant` extension: - JSON text in `utf8`/`large_utf8`/`binary`/`large_binary`, parsed with the same options as `parse_to_variant`. A NULL row is JSON null. - `struct<metadata, value>` in either field order with `binary` or `large_binary` children. Rows go through `ColumnVariantV2::EncodedRowsAppender`, which validates every row before appending and keeps a batch that shares one metadata blob in the single-metadata form. Before, each row was copied into a `VariantBatchBuilder` and then copied again into the column. - the same struct with `dictionary<integer, binary | large_binary>` metadata. It maps directly onto `ColumnVariantV2::insert_encoded_rows`: dictionary entries are the metadata blobs and indices are the metadata ids. When every row uses one entry, only that entry is passed, so no row's metadata is copied, hashed or compared. Every shape runs `ValidateFull()` on the slice and Variant metadata/payload validation before the destination changes, so a bad batch appends nothing. NULL struct rows become Variant null. 2. Adds `DataTypeVariantV2SerDe::is_arrow_input_type`. The FileScannerV2 Arrow stream reader skips `normalize_arrow_array` for a VARIANT column whose Arrow type the SerDe reads directly, so dictionary metadata reaches it encoded. `is_native_arrow_type`, which the Arrow writer uses to choose its output shape, is unchanged. Benchmark (`be/benchmark/variant/benchmark_variant_arrow_input.hpp`): one 4064-row batch of 24-field events (~1.3 KB of values per row), read into a VARIANT column, RELEASE build, before = master, after = this PR. | Input | Shared metadata | master | this PR | |---|---|---|---| | JSON text (`utf8`) | 1.7 KB | rejected | 12.3 ms | | JSON text (`utf8`) | 64 KB | rejected | 14.9 ms | | `struct<binary, binary>` | 1.7 KB | 39.0 ms | 6.3 ms (6.2x) | | `struct<binary, binary>` | 64 KB | 531 ms | 52.9 ms (10x) | | dictionary metadata, as the stream reader reads it | 1.7 KB | 41.3 ms (decoded first) | 6.0 ms (6.9x) | | dictionary metadata, as the stream reader reads it | 64 KB | 620 ms (decoded first) | 6.2 ms (100x) | Medians of 10 runs on one core of a c7i.8xlarge. On master, the dictionary row is normalize_arrow_array followed by the SerDe, which is what the stream reader did. In this PR the reader passes it through. The plain struct with 64 KB metadata is still the slowest binary form, because every row's copy of the metadata has to be compared to find that it is shared; dictionary metadata avoids that. ### Release note Stream Load with `format=arrow` can load VARIANT columns from JSON text (`utf8`, `large_utf8`, `binary`, `large_binary`) or from Parquet Variant binary `struct<metadata, value>` (with or without the `arrow.parquet.variant` extension, `binary` or `large_binary` children, either field order, and the `metadata` child optionally dictionary encoded). ### Check List (For Author) - Test: - Unit Test: `DataTypeVariantV2SerdeArrowInputTest` (12 new cases), with the existing `DataTypeVariantV2Serde*` and `ArrowFlightVariantTest` suites (59 tests, ASAN) - Regression test: `load_p0/stream_load/test_arrow_stream_load_variant`, run against a RELEASE FE/BE built from this branch (`.out` generated by the run) - Benchmark: before/after numbers above - Behavior changed: Yes. A VARIANT column in an Arrow Stream Load now accepts the Arrow types above instead of failing. Text that is not valid JSON fails the load, as other Arrow conversion errors do. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
…parse JSON text like JSON loads ### What problem does this PR solve? Issue Number: close apache#68890 Related PR: apache#69767 Problem Summary: Review follow-ups for the Arrow VARIANT reader added earlier in this PR: 1. A struct whose `metadata` child is dictionary encoded could carry an empty `value` cell, or an empty or NULL dictionary entry. `ValidateFull` accepts both, and the reader passed them to `ColumnVariantV2::insert_encoded_rows`, whose offset check is a `DORIS_CHECK` (abort on Debug/ASAN builds, internal error on Release). The reader now rejects them with INVALID_ARGUMENT naming the row or entry, as the plain binary struct path already did. 2. Arrow JSON text was parsed with `parse_to_variant`'s options, so one non-JSON string or a key over 255 bytes failed the whole load, while a JSON-format load of the same rows succeeds. It now uses the options of the Variant JSON deserializer, the same as a JSON load: text that is not JSON is kept as a string. 3. Tests: unit cases for empty values, empty/NULL dictionary entries (used and unused), and JSON-load parity; the regression suite now prints `v IS NULL`, loads a two-entry dictionary, and checks that a load with an empty value fails and loads nothing. 4. The stream reader comment notes that a VARIANT nested in ARRAY, MAP or STRUCT is still normalized with its parent. ### Release note None ### Check List (For Author) - Test: - Unit Test: `DataTypeVariantV2SerdeArrowInputTest` (13 cases) with the existing `DataTypeVariantV2Serde*` and `ArrowFlightVariantTest` suites - Regression test: `load_p0/stream_load/test_arrow_stream_load_variant` - Behavior changed: Yes. Arrow JSON text that is not valid JSON is stored as a string, as in a JSON load, instead of failing the load. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: close apache#68890 Related PR: apache#69767 Problem Summary: The benchmark header and its row generator described the data as events from event sources. The data is generic JSON objects with a configurable number of distinct keys; describe it that way. No change to the generated data or the measurements. ### Release note None ### Check List (For Author) - Test: No need to test (comment and helper rename only; clang-format checked) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
/review |
Contributor
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of PR 69767 at exact head 92b4c22 found no actionable defects and no existing P0/P1 thread to reconfirm. The change adds Arrow VARIANT JSON-text and Parquet-variant carriers, including large-binary and dictionary metadata, while preserving nested normalization. Goal and scope: the new SerDe dispatch, stream-reader binding, benchmark, unit tests, fixtures, and regression suite are focused on that behavior and the traced callers support the intended forms. Data correctness: requested row ranges are sliced logically; struct nulls become the canonical Variant null; metadata IDs and large offsets are checked; payload and metadata validation happen before ColumnVariantV2 mutation, and borrowed Arrow buffers are copied before the batch can be released. Error handling: invalid shapes, null children, dictionary bounds, empty bytes, malformed payloads, and oversized buffers return status through the exception boundary without partial destination appends. Memory/lifecycle: direct views live only through the synchronous append and the destination owns copied bytes; the scanner path is single-threaded for the batch and no new shared lock or lifecycle dependency was found. Compatibility: no Thrift/protobuf or storage-format change; top-level accepted carriers retain their wire type, while nested ARRAY/MAP/STRUCT paths continue recursive normalization. Parallel callers, nullable wrappers, extension unwrapping, benchmark registration, recursive BE test discovery, ordered regression queries, and failure-preservation coverage were checked. No new configuration or observability change applies. User focus was empty. Static-only limit: builds, unit tests, regression tests, and runtime parsing of the generated Arrow fixtures were not executed, as required by the review contract; nested end-to-end carrier coverage is a test gap without a demonstrated incorrect outcome. All assigned subagents returned NO_NEW_VALUABLE_FINDINGS in round 1, so the review converged.
Review converged after 1 round(s).
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69767 Problem Summary: The performance pipeline fails to compile the Variant Arrow JSON reader with GCC 15.1.0 and -Werror. GCC reports the function-local static constexpr json_null as unused even though the generic lambda references it. Construct the string_view directly from the null string literal instead of introducing that local constant. Arrow NULL rows still encode as JSON null, and non-NULL rows keep their existing text. ### Release note None ### Check List (For Author) - Test: - Manual test: An isolated reproducer extracted from the original reader fails with the same unused-but-set-variable error on GCC 15.2.0. The fixed reader compiles with -std=gnu++20 -O3 -Wall -Wextra -Werror and passes NULL/non-NULL input checks for 32-bit and 64-bit array views. - Manual test: The fixed isolated reader also compiles and passes the same input checks with Apple Clang 21.0.0. - Formatting: Project clang-format and check-format scripts with version 16, scoped to the modified file; git diff --check. - Full BE build, BE unit tests and regression tests were not run; this worktree has no configured BE build directory or compilation database. - Behavior changed: No - Does this need documentation: No
Contributor
|
run performance |
Contributor
TPC-H: Total hot run time: 28914 ms |
Contributor
TPC-DS: Total hot run time: 150958 ms |
Contributor
ClickBench: Total hot run time: 24.19 s |
Contributor
|
run buildall |
Contributor
TPC-H: Total hot run time: 28635 ms |
Contributor
TPC-DS: Total hot run time: 151039 ms |
Contributor
ClickBench: Total hot run time: 24.2 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…68890 Resolve the benchmark_main.cpp include conflict: master moved the full benchmark list under #ifndef DORIS_BENCHMARK_HEADER; keep benchmark_variant_arrow_input.hpp in that list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…68890 Resolve the arrow_stream_reader.cpp include conflict: keep both core/data_type_serde/arrow_parent_nulls.h (master) and data_type_variant_v2_serde.h (this PR). The VARIANT normalization skip and master's per-batch mask_arrow_descendants_not_visible() pass compose: the mask treats extension and dictionary arrays as leaves, and the Variant reader reads no child of a NULL struct row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 problem does this PR solve?
Issue Number: close #68890
Related PR: #68883
Problem Summary:
Stream Load with
format=arrowcould not load JSON text into a VARIANTcolumn, and loaded the Parquet Variant binary forms slowly.
DataTypeVariantV2SerDe::read_column_from_arrowaccepted only the exactshape Doris writes for Arrow Flight,
struct<metadata: binary, value: binary>in that field order. Through Stream Load:
utf8,large_utf8,binary,large_binary), failed;by name) failed;
large_binarychildren and a dictionary-encodedmetadatachild loaded,but only because FileScannerV2's Arrow stream reader first normalized them
to plain
binary(normalize_arrow_array).That normalization is what makes dictionary metadata expensive. A producer
that encodes a record batch at a time has one metadata blob per batch, which
holds every key of the batch, so it grows with the number of distinct keys
and can be far larger than a row's value bytes. As a plain binary child, that
blob is repeated in every row. As
dictionary<int32, binary>with one entryit costs a few bytes per row on the wire, but the reader decoded it back into
a per-row copy, and the SerDe then validated every row's copy of the
metadata.
This PR:
arrow.parquet.variantextension:utf8/large_utf8/binary/large_binary, parsed likethe Variant column of a JSON load (
serde_json_options(): text that isnot JSON is kept as a string). A NULL row is JSON null.
struct<metadata, value>in either field order withbinaryorlarge_binarychildren. Rows go throughColumnVariantV2::EncodedRowsAppender, which validates every row beforeappending and keeps a batch that shares one metadata blob in the
single-metadata form. Before, each row was copied into a
VariantBatchBuilderand then copied again into the column.dictionary<integer, binary | large_binary>metadata. It maps directly onto
ColumnVariantV2::insert_encoded_rows:dictionary entries are the metadata blobs and indices are the metadata
ids. When every row uses one entry, only that entry is passed, so no
row's metadata is copied, hashed or compared.
Every shape runs
ValidateFull()on the slice and Variant metadata/payloadvalidation before the destination changes, so a bad batch appends
nothing; empty values and empty or NULL dictionary entries are rejected
with INVALID_ARGUMENT. NULL struct rows become Variant null.
DataTypeVariantV2SerDe::is_arrow_input_type. The FileScannerV2Arrow stream reader skips
normalize_arrow_arrayfor a top-level VARIANTcolumn whose Arrow type the SerDe reads directly, so dictionary metadata
reaches it encoded. A VARIANT nested in ARRAY, MAP or STRUCT is still
normalized with its parent.
is_native_arrow_type, which the Arrow writer uses to chooseits output shape, is unchanged.
Benchmark (
be/benchmark/variant/benchmark_variant_arrow_input.hpp): one4064-row batch of 24-field JSON objects (~1.2 KB of JSON text per row), read into a
VARIANT column, RELEASE build, before = master, after = this PR.
utf8)utf8)struct<binary, binary>struct<binary, binary>Medians of 10 runs on one core of a c7i.8xlarge. On master, the
dictionary row is normalize_arrow_array followed by the SerDe, which is
what the stream reader did. In this PR the reader passes it through. The
plain struct with 64 KB metadata is still the slowest binary form, because
every row's copy of the metadata has to be compared to find that it is
shared; dictionary metadata avoids that.
End to end: Stream Load of the same 1M rows (24-field JSON objects, ~1.1 KB
each) into
(id INT, v VARIANT), one FE and one BE, RELEASE build of this PR,8 buckets, one load at a time, no compression, average of 3 runs (for the
struct<binary, binary>row, of the last 2: the first took 12.4 s, which lookslike a cold-cache outlier, though I did not confirm the cause). BE CPU is
the BE process's CPU time during the load. The stored values are identical
for every input form (same checksum of
cast(v AS STRING)).format=json,read_json_by_line)struct<binary, binary>format=json,read_json_by_line)Most of the BE CPU is the write path, which is the same for every input form,
so skipping the JSON parse with binary input saves about 8% of BE CPU and
11-14% of load time here. Arrow JSON text costs slightly more than a JSON
load; it is for clients that already produce Arrow, not a speedup.
Release note
Stream Load with
format=arrowcan load VARIANT columns from JSON text(
utf8,large_utf8,binary,large_binary), and from Parquet Variantbinary
struct<metadata, value>in either field order. Binary input withlarge_binarychildren or a dictionary-encodedmetadatachild, whichalready loaded, is now read without first being decoded to plain binary.
Check List (For Author)
DataTypeVariantV2SerdeArrowInputTest(13 new cases), withthe existing
DataTypeVariantV2Serde*andArrowFlightVariantTestsuites (59 tests, ASAN)
load_p0/stream_load/test_arrow_stream_load_variant, runagainst a RELEASE FE/BE built from this branch (
.outgenerated by the run)accepts JSON text and the swapped field order instead of failing. JSON text
is parsed like a JSON load: text that is not valid JSON and integer
literals beyond 64 bits are stored as strings, and
variant_max_json_key_lengthdoes not apply.🤖 Generated with Claude Code