Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
### What problem does this PR solve? Related PR: apache#68297 Move VARBINARY collection restrictions from the generic type coercion utility into each function's legality check. Preserve rejection before implicit casts, nested array and variadic argument coverage, existing error messages, and branch-specific CollectSet argument behavior. ### Release note None ### Check List (For Author) - Test: 17 FE unit tests passed via run-fe-ut.sh after a clean build; repository Checkstyle passed. - Behavior changed: No SQL behavior change; function-local validation now enforces the same restrictions. - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review found seven correctness gaps: two P1 failures in Iceberg VARBINARY identity-partition handling, three P2 nested-VARBINARY computation-fence bypasses, and two P2 TIMESTAMPTZ offset-range/round-trip inconsistencies. The required expression-legality checkpoint was satisfied: the changed legality overloads use getArguments()/getArgument() as directed by the local AGENTS.md. No additional user focus was provided. Three review rounds covered all 51 changed paths and relevant call chains; the final normal and risk-focused passes returned no new valuable findings, and every candidate was accepted, merged, or dismissed with evidence. Per the review instructions, this is static inspection only: I did not run builds or tests.
| buffer[len++] = ':'; | ||
| buffer[len++] = static_cast<char>('0' + offset_mins / 10); | ||
| buffer[len++] = '0' + offset_mins % 10; | ||
| // Historical zones can have sub-minute offsets. Dropping their seconds changes the |
There was a problem hiding this comment.
[P2] Accept every historical offset emitted here
This now preserves cctz's full historical offset, but both TIMESTAMPTZ parsers still reject any hour component above 14. For example, current tzdata gives Asia/Manila an offset of -15:56:08 before 1845, so this formatter emits that suffix and both strict and fallback parsing reject the returned value before conversion. Please separate wire-offset parsing from the narrower session fixed-zone policy (and add a round-trip case for one of these zones) so every value emitted here can be read back.
There was a problem hiding this comment.
Fixed in 7684c0c. Both strict and fallback TIMESTAMPTZ parsers now interpret historical wire offsets independently of the narrower session fixed-zone policy. Tests cover Manila (-15:56:08), Guam (-14:21), positive offsets beyond +14:00, and malformed offsets. DATE/DATETIME parsing is retained; this completes the exact-offset round-trip contract introduced by this PR.
| } | ||
| hour = std::stoi(std::string(rest)); | ||
| } | ||
|
|
There was a problem hiding this comment.
[P2] Enforce the fixed-offset endpoint bounds
The new UTC/GMT normalization accepts UTC+14:30 and GMT-12:30 because it only compares the hour, even though this code's fixed-zone contract is [-12:00, +14:00]. The new TIMESTAMPTZ seconds path has the same asymmetry and accepts -12:00:01. Please validate the total offset (or require zero minutes/seconds at both endpoints) and add boundary cases for the new aliases; historical wire offsets can be handled separately from session fixed zones.
There was a problem hiding this comment.
Fixed in 7684c0c. The new normalization validates the complete fixed offset, including endpoint minutes, and rejected offsets are not inserted into the cache as UTC. Tests cover direct parsing and cached lookup of UTC/GMT aliases and bare offsets. Historical TIMESTAMPTZ wire offsets are deliberately parsed separately, so -12:00:01 is valid on the wire rather than being subject to the session-setting range.
| Status DataTypeVarbinarySerDe::from_string(StringRef& str, IColumn& column, | ||
| const FormatOptions& options) const { | ||
| // Partition structs use the same hex representation as nested VARBINARY output. Decode it | ||
| // before appending so arbitrary bytes survive JSON transport instead of becoming NULL. |
There was a problem hiding this comment.
[P1] Accept the static-partition representation FE actually sends
In hybrid-static mode, VIcebergTableWriter calls this method with the string stored in static_partition_values, but Nereids VarBinaryLiteral.getStringValue() returns hex such as 00FF without a 0x prefix (and the legacy literal returns raw bytes). Consequently a static VARBINARY identity component fails this new check before row dispatch. Please align the FE/BE representation or accept the producer's actual encoding, and cover the writer call chain rather than only constructing 0x... directly in the SerDe unit test.
There was a problem hiding this comment.
The pre-PR VARBINARY SerDe returned NotSupported from from_string(), and the complete binary identity partition writer/commit path was already unsupported. Adding static-partition encoding support would broaden this PR.
| const doris::iceberg::PartitionField& field, const DataTypePtr& source_type) { | ||
| auto& transform = field.transform(); | ||
| // Identity/void only carry values; computed binary partition transforms are unsupported. | ||
| if (source_type->get_primitive_type() == TYPE_VARBINARY && transform != "identity" && |
There was a problem hiding this comment.
[P1] Do not exempt identity until the full writer path can carry it
A dynamic identity transform returns the original VARBINARY column, but VIcebergTableWriter::_get_iceberg_partition_value has no TYPE_VARBINARY case and throws; the subsequent get_partition_value switch cannot stringify it either. Even if those are added, FE reconstructs every partitioned file through IcebergUtils.parsePartitionValueFromString, which has no Iceberg BINARY/FIXED case. Thus every non-null identity partition still fails before commit. Please wire one reversible binary representation through BE extraction/path generation and FE ByteBuffer conversion, or reject identity here, with an end-to-end commit test.
There was a problem hiding this comment.
The binary identity extraction, path generation, and FE commit reconstruction gaps predate this PR. This follow-up does not implement binary partition writing or redesign its validation boundary.
| inline HashKeyType get_hash_key_type(const std::vector<DataTypePtr>& data_types) { | ||
| // Reject binary before the multi-key serialization fallback can enable joins or grouping. | ||
| for (const auto& type : data_types) { | ||
| if (type->get_primitive_type() == TYPE_VARBINARY) { |
There was a problem hiding this comment.
[P2] Reject VARBINARY recursively before serialized hashing
This check only sees the outer primitive type, so keys such as ARRAY<VARBINARY> and STRUCT<VARBINARY> fall through to HashKeyType::serialized; SerializedHashTableContext then serializes and hashes the whole complex column. The matching Nereids GROUP BY checks are also top-level-only. That re-enables the grouping/join computation this fence is intended to keep unsupported. Please reject VARBINARY anywhere in the key type tree (in both FE and BE), and cover nested single- and multi-key cases.
There was a problem hiding this comment.
The complex-type serialized hash paths already existed before this PR. A recursive FE/BE capability fence is broader than fixing newly introduced correctness/stability defects, and is not included.
| return creator_without_type::create_unary_arguments< | ||
| AggregateFunctionsSingleValue<Data<SingleValueDataComplexType>>>( | ||
| argument_types, result_is_nullable, attr); | ||
| case PrimitiveType::TYPE_VARBINARY: |
There was a problem hiding this comment.
[P2] Apply the aggregate ban to nested VARBINARY too
The complex-type cases above run before this direct VARBINARY rejection, so min/max(ARRAY<VARBINARY>) constructs SingleValueDataComplexType. Its comparison recursively reaches ColumnVarbinary::compare_at, enabling the ordering this change is trying to keep unsupported. The separate min_by/max_by factory has the same complex-key dispatch, so fixing only this switch leaves that sibling bypass open. Please reject VARBINARY recursively in both factories and add nested-array cases, while leaving byte-preserving aggregates such as collect_list alone.
There was a problem hiding this comment.
The existing complex aggregate dispatch and recursive comparison paths were not introduced by this PR. Extending the restriction recursively across sibling factories is outside this follow-up.
| return Status::OK(); | ||
| } | ||
| // Binary IO must not route IN through the shared string/storage predicate implementation. | ||
| if (context->get_arg_type(0)->get_primitive_type() == TYPE_VARBINARY) { |
There was a problem hiding this comment.
[P2] Reject nested VARBINARY before collection IN
This guard only runs for the scalar in function. An ARRAY<VARBINARY> operand passes Nereids' recursive comparability check, and VInPredicate routes the complex outer type to collection_in instead, bypassing this code. During fragment open that function hashes each constant through ColumnArray::update_crc_with_value, which reaches ColumnVarbinary's inherited unsupported implementation and fails the query. Please reject VARBINARY recursively before choosing scalar or collection IN (with ARRAY/STRUCT, nullable, and NOT IN coverage).
There was a problem hiding this comment.
The collection_in routing and missing nested VARBINARY CRC implementation predate this PR. Adding a recursive FE capability inventory is outside this follow-up.
### What problem does this PR solve? Related PR: apache#68301, apache#68297 The TIMESTAMPTZ binary-output and stream-load suites expected truncated historical offsets after formatting was fixed to preserve offset seconds. Set Asia/Shanghai explicitly in the relevant sessions and regenerate both result files to retain +08:05:43. Keep fixed-offset text-protocol and modern-date expectations unchanged. ### Release note None ### Check List (For Author) - Test: Both suites reproduced the original failures using the master PR CI artifact. Regenerated results through run-regression-test.sh and reran both suites in comparison mode: 2 passed, 0 failed. The isolated server default session time zone was UTC. - Behavior changed: No product behavior change; tests cover historical offset seconds independently of server defaults. - Does this need documentation: No
|
run buildall |
|
/review |
### What problem does this PR solve? Related PR: apache#68301, apache#68297 The TIMESTAMPTZ binary-output and stream-load suites expected truncated historical offsets after formatting was fixed to preserve offset seconds. Set Asia/Shanghai explicitly in the relevant sessions and regenerate both result files to retain +08:05:43. Keep fixed-offset text-protocol and modern-date expectations unchanged. ### Release note None ### Check List (For Author) - Test: Both suites reproduced the original failures using the master PR CI artifact. Regenerated results through run-regression-test.sh and reran both suites in comparison mode: 2 passed, 0 failed. The isolated server default session time zone was UTC. - Behavior changed: No product behavior change; tests cover historical offset seconds independently of server defaults. - Does this need documentation: No
There was a problem hiding this comment.
Request changes — capped/incomplete static review at exact head 6106e4bee324455711443952bd8f52817b1b9af1.
I found five additional P2 issue families: aggregate-side analysis/factory gaps; incomplete recursive ARRAY/MAP collection fencing; FE/BE TIMESTAMPTZ fold parity at local-year boundaries; comparison paths that bypass the direct predicate restriction; and crc32_internal(VARBINARY) passing analysis before the new routing-hash guard throws. The last issue surfaced in the third and maximum review round. The subsequent risk-focused checks found no further issue, but this review is therefore capped/incomplete rather than converged-complete.
I did not duplicate the seven current exact-head threads: the two P1 Iceberg identity/static-representation failures, plus the five P2 historical-offset, fixed-offset endpoint, recursive hash-key, nested min/max, and collection-IN gaps. Static tracing found no additional distinct issue in binary ownership/copy/move/destruction, decoder insertion, literal encoding, VarBinary SerDe/Hive Base64/hex handling, nullable casts, or the remaining timezone normalization paths. There was no additional user-specified focus beyond the full PR review.
This was static inspection only, as required: I did not build or run tests. The PR author reports 17 FE tests, 184 BE ASAN tests, Checkstyle, clang-format, and Groovy compilation passing; live SQL regression execution is stated as pending CI. Those are author claims, not independent validation in this review.
| public void checkLegalityBeforeTypeCoercion() { | ||
| // The BE set kernel cannot hash raw VARBINARY; reject it before implicit casts change its type. | ||
| for (Expression argument : getArguments()) { | ||
| if (argument.getDataType().isVarBinaryType()) { |
There was a problem hiding this comment.
[P2] Complete the aggregate-side VARBINARY fence
This check only sees a direct VARBINARY. ARRAY/STRUCT/MAP values containing a binary leaf therefore pass collect_set analysis and reach its INVALID_TYPE/INTERNAL_ERROR factory path. The same function-local gap remains for direct VARBINARY in ndv/approx_count_distinct, histogram, and linear_histogram: FE accepts them, but each BE creator list omits TYPE_VARBINARY. Please apply one recursive pre-coercion check to every aggregate whose factory does not support binary, including aliases/optional forms, while preserving intentional byte-carrying or explicitly supported cases such as collect_list, count, any_value, multi_distinct_count, and DataSketches HLL. Add direct and nested analysis tests.
There was a problem hiding this comment.
The nested aggregate dispatch limitations and sibling aggregate omissions predate this PR. This follow-up does not expand the recursive FE validation inventory or add aggregate support.
| } | ||
|
|
||
| static void checkNoVarBinaryArguments(ScalarFunction function) { | ||
| // Inspect original arguments before coercion can hide unsupported binary comparison/hash inputs. |
There was a problem hiding this comment.
[P2] Make the collection fence recursive and complete
This helper follows only ArrayType links and is called by only the eleven changed functions. Consequently Struct/Map binary leaves escape those callers; sibling array_sort/array_compact/array_min-max paths either compute via compare_at or fail in BE; and MAP construction, lookup, contains, and aggregation still compare/deduplicate VARBINARY keys or fail after analysis. Please factor a recursive Array/Struct/Map leaf check and apply it to every collection operation that compares, hashes, orders, deduplicates, or aggregates (including both array_sortby inputs), while preserving byte-carrying operations such as collect_list and map projection. Add direct and nested Array/Struct/Map cases.
There was a problem hiding this comment.
The cited sibling collection/map computations and dispatcher limitations already existed. A recursive policy across all collection functions would broaden the PR and is not included.
| cctz::civil_second civ = lookup_result.cs; | ||
| // UTC storage bounds do not guarantee a representable session-local year. Reject | ||
| // overflow before DateTimeV2 formatting could produce an offset-only wire value. | ||
| if (civ.year() < 0 || civ.year() > 9999) { |
There was a problem hiding this comment.
[P2] Apply this boundary to FE-folded casts too
This guard runs only in the BE formatter. A constant TIMESTAMPTZ-to-string cast is folded through TimestampTzLiteral.getStringValueInSessionTimeZone(), which performs the same session-zone conversion but formats Java's year without the [0,9999] check. For example, under +08:00 the folded maximum UTC literal can emit year 10000, while the equivalent nonconstant expression throws here (the minimum endpoint is symmetric). The new regression intentionally makes its input nonconstant, so it misses this fold-on/fold-off split. Please enforce the same bound in FE or defer these folds, and cover both modes/endpoints.
There was a problem hiding this comment.
Fixed in 7684c0c. FE string conversion now checks the same local-year bounds and declines an out-of-range fold through AnalysisException. Using CastException here would incorrectly fold non-strict casts to NULL. The new FE test covers CHAR/VARCHAR/STRING, both endpoints and cast modes, preservation of the CAST for BE evaluation, and valid UTC endpoint folding.
| using Creator = PredicateFunctionCreator<Traits>; | ||
|
|
||
| switch (type) { | ||
| case TYPE_VARBINARY: |
There was a problem hiding this comment.
[P2] Close comparison bypasses outside direct VARBINARY predicates
The FE restriction is not preserved across sibling comparison paths. Equal ARRAY operands pass supportCompare() and execute recursively through ColumnArray::compare_at; greatest/least admit a VARBINARY common type and fail only in BE scalar dispatch; and nullif invokes generic eq directly, computing ColumnVarbinary::compare_at without ComparisonPredicate analysis. Please use one recursive binary-comparability check before coercion for ordinary predicates and comparison-bearing scalar helpers, and add direct/nested equality, range, greatest/least, and nullif cases.
There was a problem hiding this comment.
The sibling comparison and scalar helper paths predate this PR. Building a common recursive comparability policy across them is outside this follow-up.
| inline uint32_t RawValue::zlib_crc32(const void* v, size_t len, const PrimitiveType& type, | ||
| uint32_t seed) { | ||
| // Reject binary even for NULL instead of reaching the default-type assertion or hash path. | ||
| if (type == TYPE_VARBINARY) { |
There was a problem hiding this comment.
[P2] Keep crc32_internal from failing after analysis
This new guard is also reached by the separately registered crc32_internal scalar. Its FE class still advertises variadic AnyDataType with no legality hook, and FunctionCrc32Internal passes every non-null argument to RawValue::zlib_crc32, so crc32_internal(VARBINARY) analyzes successfully and throws here during execution (while a NULL binary takes a different branch). Please either reject binary arguments before coercion in Crc32Internal, using getArguments(), or define and implement their routing-hash contract, with non-null and NULL coverage.
There was a problem hiding this comment.
The scalar already reached an unsupported binary hashing path before this explicit guard. Moving that failure into FE or defining binary routing-hash semantics would broaden this follow-up.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Historical TIMESTAMPTZ output could contain offsets rejected by its parsers. Parse wire offsets independently of session fixed-zone limits in both modes. Validate the complete UTC/GMT fixed offset and exclude rejected entries from the timezone cache. Decline FE string folding when the session-local year is outside the range accepted by the new BE formatter. ### Release note Preserve historical TIMESTAMPTZ round trips, reject invalid fixed offsets, and keep FE folding consistent with timestamp display bounds. ### Check List (For Author) - Test: 29 focused ASAN BE tests and 15 FE tests passed. Four BE tests and the new FE boundary test failed before the fixes. clang-format 16 and FE Checkstyle passed. - Behavior changed: Yes; enforce the wire, session offset, and local-year contracts. - Does this need documentation: No.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes — converged static review at exact head 7684c0cfd377c77399f18bac0ffaefc88540e56a.
One P1 remains: the unversioned Hive LazySimpleSerDe TEXTFILE VARBINARY change from legacy raw bytes to Base64 silently reinterprets existing Base64-looking values and is incompatible with mixed old/new readers and writers. The inline comment traces the persisted sink/file/read path and requests an explicit compatibility or migration mode with cross-version coverage.
Coverage included all 58 authoritative changed paths and 2,494 diff lines, plus relevant ownership, decoder, RPC/literal, Hive scalar and nested SerDe, capability-fence, and TIMESTAMPTZ call chains. Three complete review rounds converged; all three final-round reviewers returned no new valuable findings. I refreshed and deduplicated against every live inline discussion immediately before submission. The required pre-coercion legality hooks use getArguments()/getArgument() as directed by the local AGENTS.md. No additional user focus was provided.
This was static inspection only, as required: I did not build or run tests, and author/CI validation claims were not independently verified.
| Status DataTypeVarbinarySerDe::deserialize_one_cell_from_hive_text( | ||
| IColumn& column, Slice& slice, const FormatOptions& options, | ||
| int hive_text_complex_type_delimiter_level) const { | ||
| // Hive LazyBinary uses lenient Base64 (including URL-safe letters and whitespace), |
There was a problem hiding this comment.
[P1] Preserve Hive TEXTFILE binary rows across upgrades
This unconditionally changes Doris's persisted Hive TEXTFILE representation from raw bytes to Base64. VHivePartitionWriter writes this method's output through VCSVTransformer, and later scans feed those file bytes back through TextReader to this decoder. After an upgrade, a legacy raw value such as dGVzdDI= is therefore silently returned as test2; in a mixed-version cluster, an old reader returns a new writer's Base64 text as the value. The raw fallback cannot disambiguate legacy payloads that happen to use the Base64 alphabet. Please add an explicit table/catalog compatibility or migration mode for legacy raw files, and cover old-writer/new-reader plus new-writer/old-reader scalar and nested cases.
### What problem does this PR solve? The binary literal unit test used a StringView after the temporary owning Field was destroyed. Keep that Field alive through the assertion, covering short and long values with embedded NUL bytes. The TIMESTAMPTZ cast regression still classified +15:00 as invalid after the historical wire-offset parser fix. Use +24:00 for the rejection test and cover valid historical offsets in both cast modes with generated snapshots. ### Release note None; this only fixes tests for the existing PR behavior. ### Check List (For Author) - Test: 34 focused ASAN BE tests passed on each branch. The original lifetime bug reproduced under ASAN. The shared SQL cast suite passed in comparison mode from both branch checkouts against the master CI artifact; new snapshots were generated by the runner. clang-format 16 passed. Full clang-tidy is blocked by pre-existing diagnostics. - Behavior changed: No production behavior change. - Does this need documentation: No.
|
run buildall |
|
/review |
2 participants
What problem does this PR solve?
This is the first of five planned extractions from #67784, targeting
branch-4.1.Binary
Fieldvalues can retain references to released source storage, and Hive binary text needs its own Base64 contract. TIMESTAMPTZ output can lose historical offset seconds, format invalid NULL payloads, or fail again while reporting a boundary cast error.Arrow convertor migration, Parquet/ORC semantics, external writer changes and catalog mapping migration belong to the subsequent extractions. This PR does not enable native VARBINARY storage.
Testing
TIMESTAMPTZ regression follow-up: reproduced both binary-output and stream-load failures using the master PR CI artifact, regenerated the two snapshots through
run-regression-test.sh, and passed both suites in comparison mode from each branch checkout. ExplicitAsia/Shanghaisession settings were verified with the server default session zone set to UTC. Only historical offset seconds changed in the generated results.Function-local validation update: 17 FE tests passed after a clean build with Checkstyle enabled. Coverage includes direct legality checks, nested/mixed/variadic VARBINARY arguments, both
collect_setarities, supported ordinary types, SQL analysis, and existing array rewrites. Collection restrictions now live in each function's legality check before coercion; existing branch-specific argument rules are preserved.Rebuilt the BE ASAN test target from this extraction: 184 tests passed, zero failures. Coverage includes binary lifetime/SerDe/rejection paths, timestamp parsing/casts, hash and partition guards, and existing Arrow/Variant serialization tests.
VarBinaryUnsupportedCollectionTest: passed (13 unsupported collection expressions, plus existing byte-preserving array/collection analysis).FE reactor
validatewith repository Checkstyle: passed.clang-format 16 check on all 34 changed C++ source/header files: passed.
Groovy compilation of the three new regression suites: passed. Live SQL regression execution is pending CI.
The local BE test source list was narrowed for the focused build and restored before committing. No build configuration changes are included.
Release note
Fix binary value lifetime and serialization, reject unsupported binary computation paths, and preserve TIMESTAMPTZ historical offsets and boundary error behavior.
Check List (For Author)
Check List (For Reviewer who merge this PR)
Scoped review follow-up
This follow-up only fixes correctness/stability defects introduced by this PR. Compatibility preservation, pre-existing limitations, additional VARBINARY computation/validation, and unrelated refactors are excluded.
CI test follow-up
This follow-up changes tests only and retains the agreed scope: no compatibility work or additional binary computation support. Existing muted failures are outside this fix.