[fix](types) Fix binary value ownership and timestamp primitives - #68301
Conversation
### What problem does this PR solve? This ports apache#68297 to `master`, preserving the first of five planned extractions from apache#67784. Binary `Field` values 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. - Own long binary Field values while keeping short values inline. Preserve execution type lengths and decoder bytes, and add Hive Base64 and hexadecimal decoding support. - Explicitly reject unsupported binary hash keys, IN, aggregates, predicates and computed partition transforms. Keep the existing FE comparison/group/join restrictions and existing binary scalar functions. Reject unsupported collection kernels before coercion. - Preserve historical second offsets in both TIMESTAMPTZ formatting and parsing. Skip masked NULL payloads, reject unrepresentable local years, and preserve cast error/NULL behavior at boundaries. 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. ### Master adaptation - Retain the fixed-offset normalization and tests already present on master. - Use the current void-returning `VInPredicate::_prepare_zonemap_min_max` interface in both the guard and its test. - Retain master header cleanup and existing timestamp-nanosecond tests. - Retain the existing master binary-literal encoder and its StringView input contract; the older std::string-based caller fix is not applicable. ### Testing - BE ASAN build and **199 tests passed** across 17 suites using `run-be-ut.sh`, including binary lifetime/SerDe/rejection, timestamp parsing/casts, and existing Arrow/Variant serialization coverage. - `VarBinaryUnsupportedCollectionTest`: **passed** (13 unsupported expressions plus supported byte-preserving collection analysis). The FE test reactor and repository Checkstyle passed after cleaning stale branch build artifacts. - Repository clang-format 16 check and build-header hygiene checks: **passed**; 31 changed C++ source/header files. - Groovy compilation of the three regression suites: **passed**. Live SQL regression execution remains pending CI. - clang-tidy was attempted but could not complete because master already contains an unmatched `NOLINTEND` in `be/src/core/types.h`. A diagnostic run with the compiler resource directory corrected reproduced that blocker; the other reported findings in `column_varbinary.cpp` were outside changed lines. This is not a clean clang-tidy result. The focused BE test source list and local test/build settings were 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) - Test - [x] Regression test (three self-checking suites added; execution pending CI) - [x] Unit Test - Behavior changed: - [x] Yes. Binary rejection and timestamp boundary behavior are described above. - Does this need documentation? - [x] No. This fixes existing type behavior without introducing a configuration option. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
|
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#68301 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 the CollectSet constant-limit check. ### Release note None ### Check List (For Author) - Test: 18 FE unit tests passed via run-fe-ut.sh; repository Checkstyle passed. New direct-legality tests reproduced the missing checks before the change. - Behavior changed: No SQL behavior change; function-local validation now enforces the same restrictions. - Does this need documentation: No
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review of exact head 44eef01c38d2ee094290cfc1db07535c76ec2274 found five blocking issues: the shared VARBINARY hash guard removes previously supported serialized multi-key paths in PartitionTopN, INTERSECT/EXCEPT, and recursive UNION DISTINCT; Iceberg identity partitions are allowed even though the writer/commit path cannot represent VARBINARY; two TIMESTAMP_NS error branches re-enter the new throwing formatter; historical offsets emitted by the formatter remain outside both parsers' accepted range; and the three new deterministic regression suites do not use runner-generated golden output.
Critical checkpoints: Field ownership, copy/move/destruction, decoder-page lifetime, raw/Hive-Base64/nested-hex SerDe routing, nullable handling, and protobuf length restoration were traced without another issue. FE/BE and parallel-path review found the hash-consumer regression above; comparison/join/group fences, IN, single-value aggregates, crc32, collect-set, and collection coercion candidates were either already covered or dismissed with concrete evidence. TIMESTAMPTZ strict/non-strict parsing, local-year boundaries, NULL masking, and protocol formatting were traced; the two reported timestamp defects are the surviving error/round-trip gaps. No additional concurrency, persistence, configuration, performance, or observability defect was substantiated.
Validation is static-only: no builds or tests were run under this review constraint. Author-reported FE/BE checks and CI claims were not independently validated, and the PR description says live SQL regression execution is pending. There were no user-supplied focus points beyond the full review.
This review is capped/incomplete for convergence purposes: valuable new scope and one new candidate appeared in round 3, the maximum permitted round. All candidates currently identified have nevertheless been independently adjudicated, deduplicated, and either included below or dismissed with evidence.
| 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.
[P1] Preserve the existing serialized path for supported multi-key consumers. This pre-check now runs before the multi-key branch, so it also rejects keys that previously selected HashKeyType::serialized: default-enabled PartitionTopN windows, multi-column INTERSECT/EXCEPT, and recursive UNION DISTINCT. Their PartitionedHashMapVariants, SetDataVariants, and DistinctDataVariants all implement serialized keys, and ColumnVarbinary supplies serialization; only single-key VARBINARY was already unsupported. Please move the rejection to the join/group consumers that require it (or otherwise retain these byte-safe consumers) and add multi-key window, set-operation, and recursive-CTE coverage.
There was a problem hiding this comment.
The rejection is intentional for this change. Preserving previously reachable serialized hash consumers is a compatibility concern, so the common VARBINARY hash restriction is retained.
| 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] Reject identity until the writer supports binary partitions. This branch deliberately leaves identity enabled, but IdentityPartitionColumnTransform returns ColumnVarbinary and VIcebergTableWriter::_get_iceberg_partition_value has no TYPE_VARBINARY arm, so a non-null dynamic identity partition falls into "Unsupported type for partition". The later partition-string and FE commit reconstruction paths also have no binary-safe representation. With enable.mapping.varbinary=true, inserts into Iceberg BINARY/FIXED identity partitions therefore fail. Please either reject identity here too, or implement byte-safe extraction/transport/reconstruction and add an end-to-end identity test.
There was a problem hiding this comment.
The binary identity extraction/commit path was already incomplete before this PR. This PR does not add binary partition-writing support; completing that pipeline or redesigning its rejection boundary is outside this follow-up.
| 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] Keep the remaining TIMESTAMP_NS error paths from re-entering this throwing formatter. With the session zone +08:00, the maximum UTC TIMESTAMPTZ first fails local conversion because the year becomes 10000; strict TIMESTAMPTZ-to-TIMESTAMP_NS cast and TIMESTAMPTZ/TIMESTAMP_NS comparison then call to_string(local_zone) while building their InvalidArgument Status, so this exception escapes before that Status is returned. The parallel cast paths changed in this PR already render utc_dt() instead. Please update these two missed callers the same way and add strict-cast and comparison boundary tests.
There was a problem hiding this comment.
Fixed in 0765475. Both remaining TIMESTAMP_NS error paths now format the valid UTC value instead of calling the session-local formatter again. ASAN tests cover both year endpoints, strict errors/non-strict NULL results, and both comparison operand orders; all 31 focused BE tests passed.
| "invalid timezone offset '{}'", | ||
| combine_tz_offset(sign, part[0], part[1])); | ||
|
|
||
| if (second_offset != 0) { |
There was a problem hiding this comment.
[P2] Accept every historical offset the formatter emits. TimestampTzValue::to_string() now emits the exact cctz offset, but both parser branches still cap the hour at 14 and this seconds branch additionally caps negative offsets at 12. IANA history includes larger values (for example Asia/Manila used -15:56:08 before 1845), so Doris can return a TIMESTAMPTZ string that it cannot parse back. Please align both parser branches with the formatter's range and add strict/non-strict round-trip coverage for an extreme historical zone.
There was a problem hiding this comment.
Fixed in 0765475. 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.
| as timestamptz(6)) | ||
| from numbers('number' = '1') | ||
| """)[0][0].toString() | ||
| assertEquals(testCase[2], wire) |
There was a problem hiding this comment.
[P2] Record these deterministic results as golden output. All three new TIMESTAMPTZ suites assert successful query results directly and add no .out files, while this repository requires determined regression results to use qt_/order_qt_ cases with runner-generated output. Please keep the test { sql; exception } blocks for failures, convert the successful cases in all three suites, and generate their checked-in .out files.
There was a problem hiding this comment.
These suites already assert their results. Converting all three suites to golden-output tests is a test-style change, not a newly introduced correctness or stability fix, so they are not rewritten in this follow-up.
TPC-H: Total hot run time: 28080 ms |
TPC-DS: Total hot run time: 152721 ms |
ClickBench: Total hot run time: 23.84 s |
### 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
### 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 |
TPC-H: Total hot run time: 27623 ms |
TPC-DS: Total hot run time: 152870 ms |
ClickBench: Total hot run time: 23.87 s |
There was a problem hiding this comment.
Request changes — exact-head static review of f68c0babcebd1ef82e9804e26900b4357efd99ab against the authoritative merge-base diff. I found 13 new, distinct issue groups (3 P1, 10 P2) and attached them inline. The existing live threads covering the blanket multi-key hash rejection, Iceberg identity partitions, extreme historical-offset parsing, two missed TIMESTAMP_NS/error-formatting callers, and missing regression golden files remain valid duplicate fences and are intentionally not repeated here.
Critical checkpoint conclusions:
- Goal and correctness: not yet met. The Field ownership and PType-length work is coherent, but top-level Hive partition parsing is broken, TIMESTAMPTZ still has parallel formatter/folding inconsistencies, and the advertised unsupported-VARBINARY boundary remains incomplete across FE/BE entry points.
- Scope and clarity: the local changes are generally focused, but the function-by-function rejection design leaves numerous public scalar and aggregate paths FE-legal and BE-failing, and it rejects one supported arity of
array_enumerate_uniq. - Concurrency, lifecycle, configuration, and persistence: no new concurrency, lock-order, dynamic-configuration, transaction, EditLog, or static-initialization issue was found. The new owning Field and decoder-copy lifetimes were traced through copy/move/destruction and appear safe.
- Compatibility and parallel paths: serialized VARBINARY length restoration is sound, and no new storage-format incompatibility was found. Parallel evaluator, formatter, literal-folding, connector, scalar-arity, and aggregate-factory paths are not aligned; the inline comments identify the concrete failures.
- Data writes/connectors: the new Hive partition-materialization failure is reported inline. The separate Iceberg identity-write failure is already covered by a live thread.
- Error/null/performance: nullable TIMESTAMPTZ correctness improves, but the generic fix regresses all sparse-null cast-to-string blocks to row-wise virtual formatting. Error/format behavior also remains inconsistent in the reported parallel paths.
- Tests: the PR adds useful unit coverage for ownership, SerDe, parser, and boundary behavior, but it misses the newly reported FE/BE and session-path cases and asserts rejection for the supported multi-array
array_enumerate_uniqbranch. The existing live thread already covers the three assertion-only regression suites and missing runner-generated.outfiles. - Observability and FE/BE variables: no additional metrics/logging or new variable transport is required. The existing
short_circuit_evaluationsetting does, however, expose inconsistent correctness for VARBINARY conditional expressions. - User focus: no additional focus was supplied; the full changed-file set and relevant call chains were reviewed.
- Convergence: the third and final permitted round found
linear_histogram,ndv/approx_count_distinct, and the multi-arrayarray_enumerate_uniqregression; these are included in the aggregate-inventory and arity-specific comments. Under the review contract this makes the review capped/incomplete; no fourth discovery round was permitted.
Validation is static only: I did not run builds or tests. At submission time, GitHub reports compile, Clang format, CheckStyle, license, performance, and vault P0 passing; BE UT is failing (the TeamCity details are authentication-gated), while FE UT and several regression lanes remain pending. Those CI results are not independent validation of the findings above.
| private ArrayFunctionUtils() { | ||
| } | ||
|
|
||
| static void checkNoVarBinaryArguments(ScalarFunction function) { |
There was a problem hiding this comment.
[P2] Cover the remaining unsupported collection entry points with this pre-coercion check. array_except_all, array_min, and array_max still accept ARRAY<VARBINARY> in Nereids: array_except_all then reaches dispatch_switch_all, which has no VARBINARY case, while array_min/array_max reach the newly added VARBINARY throw in the single-value aggregate factory. Please invoke this guard from those three functions and add them to the FE rejection tests so these queries fail during analysis instead of BE preparation/execution.
There was a problem hiding this comment.
These operations were already unsupported in BE. Moving their rejection into FE would broaden the function-validation work and is not needed to fix a newly introduced execution defect.
| 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] Add the matching FE rejection for every aggregate routed here. Nereids still accepts min(VARBINARY), max(VARBINARY), and any_value(VARBINARY) (Min/Max only reject metric types and AnyValue has an unrestricted signature), but all three names are registered through this factory and now throw while the BE builds the aggregate. Please reject them before coercion and add analysis tests for the public names.
There was a problem hiding this comment.
These aggregate inputs were already unsupported; the new BE branch reports that unsupported case explicitly. A broader FE rejection inventory 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 this type in Nereids as well. InPredicate.checkLegalityBeforeTypeCoercion excludes object/complex types, and supportCompare accepts VARBINARY as an ordinary primitive, so same-typed IN/NOT IN expressions still plan successfully and fail only when this BE function opens. Please add the pre-coercion check and same-/mixed-type analysis coverage.
There was a problem hiding this comment.
VARBINARY IN/NOT IN remains unsupported. Moving the existing unsupported-type failure to FE is an analysis/diagnostic improvement rather than a new correctness fix.
| // Inspect original arguments before coercion can hide unsupported binary comparison/hash inputs. | ||
| for (Expression argument : function.getArguments()) { | ||
| DataType type = argument.getDataType(); | ||
| while (type instanceof ArrayType) { |
There was a problem hiding this comment.
[P2] Apply the same restriction to the map membership entry points. map_contains_key, map_contains_value, and map_contains_entry have broad Nereids signatures and no legality check, but the first two forward to the array-index dispatcher and the third uses its own dispatch_switch_all; neither dispatches VARBINARY. Add equivalent recursive checks for VARBINARY keys/values and FE analysis coverage.
There was a problem hiding this comment.
The referenced map-membership dispatchers already lacked VARBINARY support before this PR. Expanding function-local FE validation is outside this follow-up.
| 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] Preserve the top-level partition-string contract here. With enable.mapping.varbinary=true, HiveScanRange copies each non-null HMS partition value directly into columns_from_path, and FileScannerV2::_parse_partition_value calls this from_string; any ordinary value not starting with 0x now fails with Invalid VARBINARY hex representation. The 0x grammar is symmetric only with nested serialization (to_string emits it at nesting level >= 2), so please restrict hex decoding to that context or use a dedicated nested decoder, and add a Hive partition scan test.
There was a problem hiding this comment.
The top-level and nested encodings differ, but the pre-PR VARBINARY SerDe inherited from_string() returning NotSupported. Thus this is not a regression of a previously supported Hive partition decoder. Adding that decoder is outside the current primitive changes.
| 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] Fence the SQL caller as well. crc32_internal is a registered builtin whose Nereids class accepts variadic AnyDataType, and its BE implementation calls RawValue::zlib_crc32 for every non-null argument, so crc32_internal(VARBINARY) now plans successfully and throws here at execution. Please reject VARBINARY in Crc32Internal before coercion (with an FE analysis test), or implement the byte hash if the debug scalar should support it.
There was a problem hiding this comment.
crc32_internal VARBINARY was already unsupported before the explicit BE rejection. Adding another FE function guard or a binary hash implementation is outside this follow-up.
| } else if (primitive_type == TYPE_AGG_STATE) { | ||
| // Do nothing | ||
| nested = std::make_shared<DataTypeAggState>(); | ||
| } else if (primitive_type == TYPE_VARBINARY) { |
There was a problem hiding this comment.
[P2] Complete execution support for this newly reconstructible type in the default conditional evaluators. With the default short_circuit_evaluation=false, a multi-row coalesce whose selected values come from different arguments reaches filled_result_column, where dispatch_switch_scalar omits VARBINARY; a two-or-more-WHEN CASE separately reaches VCaseExpr::_execute_update_result, whose type switch also omits it. Both expressions are FE-legal, and both work when short-circuit evaluation is enabled through generic insert_from, so results currently depend on this session setting. Please add the generic/VARBINARY paths and regression cases for mixed-row COALESCE and multi-branch CASE under both settings.
There was a problem hiding this comment.
The normal COALESCE/CASE dispatch omissions and the alternate short-circuit paths predate this PR. The protobuf datatype reconstruction change does not introduce those evaluator branches. Adding VARBINARY execution support is outside this follow-up.
| while (type instanceof ArrayType) { | ||
| type = ((ArrayType) type).getItemType(); | ||
| } | ||
| if (type.isVarBinaryType()) { |
There was a problem hiding this comment.
[P2] Include the remaining scalar ordering entry points in the pre-coercion rejection sweep. least and greatest preserve VARBINARY as their common type, but with two or more arguments the BE creates a ColumnVarbinary, misses the string branch, and reaches dispatch_switch_scalar, which has no VARBINARY case. Please add a shared legality check for both names and two-argument analysis tests; the deliberate unary passthrough can remain supported if desired.
There was a problem hiding this comment.
The multiple-argument VARBINARY ordering dispatcher was already unsupported. Adding FE validation or preserving the unary compatibility path is outside this follow-up.
| AggregateFunctionsSingleValue<Data<SingleValueDataComplexType>>>( | ||
| argument_types, result_is_nullable, attr); | ||
| case PrimitiveType::TYPE_VARBINARY: | ||
| // Owning binary values for IO must not implicitly enable single-value aggregates. |
There was a problem hiding this comment.
[P2] Extend the FE-before-BE rejection inventory beyond this factory. The same mismatch remains for min_by/max_by with a VARBINARY ordering key, group_array_intersect/group_array_union with ARRAY<VARBINARY>, and VARBINARY inputs to histogram/hist, linear_histogram, topn_array, ndv/approx_count_distinct, map_agg_v1, map_agg_v2, and alias map_agg: their Nereids signatures admit the type, while the corresponding BE creator dispatches omit it and return no function. Please add function-local pre-coercion checks and analysis coverage for these public names, or add BE support where byte ordering/copying is intended.
There was a problem hiding this comment.
These dispatcher omissions predate this PR. Completing the aggregate/alias validation inventory or introducing new BE support would broaden the change, so neither is included.
| */ | ||
| @Override | ||
| public void checkLegalityBeforeTypeCoercion() { | ||
| ArrayFunctionUtils.checkNoVarBinaryArguments(this); |
There was a problem hiding this comment.
[P2] Restrict this rejection to the one-array form. The unary BE branch uses dispatch_switch_scalar and cannot handle VARBINARY, but with two or more arrays FunctionArrayEnumerateUniq directly selects MethodSerialized; that path serializes every nested column, and this PR supplies the required serialization methods for ColumnVarbinary. The blanket guard therefore turns a supported multi-array form into an analysis error. Please preserve that arity and replace the mixed-array rejection assertion with execution coverage for long, empty, embedded-NUL, duplicate, and nullable binary tuple components.
There was a problem hiding this comment.
Preserving the multi-array form is a compatibility request and is not part of this follow-up. Also, ColumnVarbinary serialization methods already existed before this PR; they were not introduced by it. The current rejection remains.
FE UT Coverage ReportIncrement line coverage |
### 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. The new local-year formatter exception also interrupted TIMESTAMP_NS cast and comparison error reporting; render UTC values when constructing those errors. ### Release note Preserve historical TIMESTAMPTZ round trips and timestamp conversion errors. ### Check List (For Author) - Test: 31 focused ASAN BE tests passed, including ordinary date/datetime parsing; three targeted tests failed before the fixes. clang-format 16 and build hygiene passed. Full clang-tidy remains affected by pre-existing diagnostics. - Behavior changed: Yes; accept historical wire offsets and preserve InvalidArgument/NULL handling. - Does this need documentation: No.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of exact head 0765475366abe60678176acbbb82ef7d34366bec against base 695c88b57723ffd93c6a0978daa8725cb21b2562. I found two new actionable issues after deduplicating against all live inline threads and replies.
Checkpoint conclusions:
- Objective, scope, and focus: reviewed all 55 authoritative changed paths and the relevant VARBINARY/TIMESTAMPTZ call chains; there was no additional user-provided focus. The implementation intent is clear, but the widened offset parser invalidates an existing checked-in regression expectation.
- Concurrency, locking, lifecycle, and static initialization: no new shared-state or locking path is introduced. The
Fieldowner, copy/move/assignment/destruction paths and decoder lifetimes were traced; no distinct lifetime defect remained. No relevant static-initialization issue was found. - Configuration dynamics: strict/non-strict casts, session time zones, folding, nullable shapes, and session-state restoration were traced. The nullable batch-performance concern is already covered by an existing thread; no additional configuration-dependent correctness issue remained.
- Compatibility and rolling behavior: protobuf VARBINARY length reconstruction preserves declared/default length, and no persisted/replay format changes were found. Historical TIMESTAMPTZ text can now be 35 bytes, but MySQL result metadata still advertises 32; see the inline finding.
- Parallel paths and special checks: strict/fallback parsers, central/ISO/FE formatters, text/prepared MySQL rows, Arrow, hash consumers, aggregates, predicates, Iceberg, collection aliases, and FE legality were reconciled. The surviving parallel-path issues already have live threads and were not duplicated.
- Tests and results: one pre-existing golden will now deterministically fail at
+15:00; the separate missing-golden convention issue already has a live thread. This review was static only: no build or test was run. Author-reported test results are not independent validation. At submission time, formatter/checkstyle/license/title checks pass, while compile, FE/BE unit tests, performance, and automated review checks remain pending. - Observability and error handling: the reviewed head fixes the prior throw-while-formatting TIMESTAMP_NS errors; no new distinct diagnostic/observability issue remained.
- Persistence, transactions, and data writes: no new replay, atomicity, or transaction defect was found. The Iceberg binary identity path is already covered by a live thread.
- FE/BE contracts: remaining VARBINARY over-rejections and missing FE-before-BE gates are already covered by live threads; no new entry point survived deduplication.
- Performance: no new distinct issue beyond the already-threaded nullable batch deoptimization.
- Other correctness: no additional finding survived two review rounds and the final changed-file sweep.
| } | ||
| SET_PARAMS_RET_FALSE_IFN(part[0] <= 14, "invalid hour offset '{}'", part[0]); | ||
| SET_PARAMS_RET_FALSE_IFN( | ||
| part[0] < (type == DataTimeCastEnumType::TIMESTAMP_TZ ? 24U : 15U), |
There was a problem hiding this comment.
[P1] Update the existing +15:00 regression expectation. This widened range now accepts +15:00 in both parsers (and the new unit test explicitly treats it as valid), but test_cast_timestamptz.groovy still includes +15:00 in its non-strict invalid row with a checked-in NULL in test_cast_timestamptz.out. Because that suite disables constant folding, it reaches this BE branch and will return a value instead of NULL. Move +15:00 to the valid cases (retain +24:00 as the rejection boundary) and regenerate the golden output.
| buffer[len++] = '0' + offset_mins % 10; | ||
| // Historical zones can have sub-minute offsets. Dropping their seconds changes the | ||
| // instant represented by the client-visible wall clock and offset when read back. | ||
| const int offset_seconds = abs_offset % 60; |
There was a problem hiding this comment.
[P2] Keep MySQL column metadata wide enough for the new format. With scale 6, appending :SS produces a 35-byte value (YYYY-MM-DD HH:MM:SS.ffffff+HH:MM:SS), and both text and prepared-result paths send that full string. MysqlSerializer.getMysqlTypeLength(TIMESTAMPTZ), however, still advertises 32 bytes based on +HH:mm, so clients see a display width smaller than rows this formatter now emits. Please raise the TIMESTAMPTZ metadata width to 35 and cover the serialized Column Definition value.
TPC-H: Total hot run time: 27685 ms |
TPC-DS: Total hot run time: 153045 ms |
ClickBench: Total hot run time: 24.16 s |
### 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 original SQL failure reproduced on the master CI artifact; the full corrected cast suite passed in comparison mode with generated new snapshots. 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 |
TPC-H: Total hot run time: 27949 ms |
TPC-DS: Total hot run time: 152852 ms |
ClickBench: Total hot run time: 24.15 s |
There was a problem hiding this comment.
Static review conclusion: request changes for one newly introduced stability issue.
The VARBINARY lifetime, decoder-copy, Hive/nested SerDe, protobuf-length, explicit capability-gate, and TIMESTAMPTZ parser/formatter/null-boundary changes are otherwise internally coherent. Copy/move/destruction and text/prepared TIMESTAMPTZ paths were traced; no new concurrency, locking, configuration, persistence, transaction, data-write sequencing, or FE-BE propagation issue was found. Compatibility and parallel-path concerns that remain are already covered by current inline threads and were not duplicated.
Blocking point: long VARBINARY Field ownership allocates through raw new[], skipping Doris task-tracker accounting and the checked pre-allocation failure path.
Tests/observability: changed tests cover lifetime, decoder, SerDe, malformed data, historical offset parsing/formatting, nullable behavior, boundaries, and protocol paths; errors remain actionable. This review was static only as required: no builds or tests were run, and author/CI results were not independently validated.
No additional user-provided focus was present.
| view = StringView(bytes, value.size()); | ||
| } | ||
| OwnedBinaryField(const OwnedBinaryField&) = delete; | ||
| OwnedBinaryField& operator=(const OwnedBinaryField&) = delete; |
There was a problem hiding this comment.
[P1] Allocate this retained payload through Doris's checked allocator. Every non-inline VARBINARY value materialized as a Field now duplicates value.size() bytes with raw new[]; this is reached by ordinary column extraction plus Iceberg defaults and Parquet metadata, and the declared length can be Integer.MAX_VALUE. The current jemalloc hook only routes allocation calls and does not consume/release the task tracker, while raw new[] also skips Allocator::memory_check and its controlled MEM_ALLOC_FAILED path. A large value (and the temporary/destination deep-copy peak) can therefore exceed a query or process limit without Doris charging it or rejecting it at the configured limit. Please use DorisUniqueBufferPtr or an equivalent allocator-backed owner that still fits Field::storage, and cover allocation/destruction under a limited tracker.
There was a problem hiding this comment.
Fixed in 221207e and synchronized to #68297 (0a220b4).
Long binary payloads now use Allocator for checked allocation and release. The RAII owner remains standard-layout and fits Field::storage, preserving the leading StringView access. It stores the allocation length separately because Field::get() exposes a mutable view. Inline payloads remain allocation-free.
Added limited-tracker tests for retained/copy accounting, replacement and destruction, oversized allocation rejection, and failed copy/assignment preserving the original value. Both new tests failed with the original new[] implementation. All 40 focused ASAN BE tests pass on each branch.
Also stabilized the failing historical-offset fixture with fixed zones so the large negative offsets and second-precision round trips do not depend on the host's historical Manila/Guam tzdata. This change stays within the existing PR scope.
clang-format 16 and master build hygiene passed. Full clang-tidy remains blocked by pre-existing diagnostics, including the unmatched NOLINTEND in core/types.h. CI has been retriggered on both PRs.
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Long binary Fields retained payloads with raw new[], bypassing Doris memory tracking and allocation checks. Use an allocator-backed standard-layout owner that fits the existing Field storage. Release the original allocation size even if the exposed view changes, and preserve existing values on failed replacement. Historical Manila offsets differ across tzdata versions. Use fixed zones for large negative offset fixtures while retaining second-precision round trips. ### Release note Retained binary Field payloads now use Doris memory accounting and limit checks. ### Check List (For Author) - Test: 40 focused ASAN BE unit tests passed. Both new memory tests failed against the original allocator path. clang-format 16, build hygiene and diff checks passed. Full clang-tidy is blocked by pre-existing diagnostics. - Behavior changed: Yes, retained binary payloads use checked allocations. - Does this need documentation: No.
|
run buildall |
|
/review |
TPC-H: Total hot run time: 27795 ms |
TPC-DS: Total hot run time: 152387 ms |
ClickBench: Total hot run time: 24.03 s |
|
|
||
| void ColumnVarbinary::insert_many_continuous_binary_data(const char* data, const uint32_t* offsets, | ||
| size_t num) { | ||
| reserve(size() + num); |
There was a problem hiding this comment.
这个reserve 是错的。 不是 +num，应该是计算一下offsets【num】 - 0 ？
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Static exact-head review of 221207ec3a1ae824b81657a1ea4dbb9b8bd51fb0. I found five blocking correctness/compatibility issues: the Hive BINARY read contract is neither propagated nor matched, Hive binary text conversion stages unadmitted full-cell buffers, several previously valid mixed array calls are rejected before coercion, BE and FE disagree at a fractional-carry/year boundary, and the new seconds-offset formatter output is rejected by FE catalog literal parsing.
Checkpoint summary: I reviewed all 58 changed paths and relevant upstream/downstream call chains, including ownership and failure rollback, legacy/v2 Hive text paths, storage/Parquet copying, collection signature resolution and BE dispatch, both TIMESTAMPTZ parsers, FE folding/catalog parsing, MySQL output, error/NULL paths, and persistence/configuration boundaries. The allocator-backed hidden Field owner is lifecycle- and tracker-balanced; its remaining move/deep-copy peak is already covered by an existing inline thread. No additional concurrency, static-initialization, transaction, or persisted-format defect survived deduplication. Existing MySQL-width, ISO8601, shared hash-key, partition-hex, collection-inventory, and nullable-performance threads were treated as hard duplicate fences.
Validation is static only: the authoritative review bundle prohibited builds and tests. Author/CI results were not treated as independent execution evidence. Targeted coverage is still needed for Hive raw/versioned decoding under memory limits, supported mixed-type coercions, folded/non-folded boundary rounding, and formatter-to-FE-default round trips.
| return Status::OK(); | ||
| } | ||
|
|
||
| Status DataTypeVarbinarySerDe::deserialize_one_cell_from_hive_text( |
There was a problem hiding this comment.
[P1] Propagate the Hive BINARY decoding contract instead of guessing it from each cell. Hive 4 supports hive.serialization.decode.binary.as.base64=false for raw TEXTFILE values, but FE drops that property, so a configured raw value such as test is silently Base64-decoded here to different bytes. In default mode Hive 4/current LazyBinary also uses the strict Basic decoder with raw fallback, while this code accepts URL-safe letters/whitespace and truncates a one-sextet tail (for example Hive preserves -_8= but Doris returns 0xfb 0xff). Please pass the property/version-selected mode through TFileAttributes, use the matching decoder, and cover raw plus Hive 3/4 Base64 cases. See the Hive TEXTFILE contract and Hive 4 LazyBinary.
| // Hive LazyBinary uses lenient Base64 (including URL-safe letters and whitespace), | ||
| // falling back to the original bytes for non-Base64 input or an empty decoding. | ||
| // Keep this separate from JSON/CSV: those formats do not share Hive's encoding contract. | ||
| std::string encoded; |
There was a problem hiding this comment.
[P1] Put these full-cell Hive staging buffers behind Doris's checked allocator. For every valid binary field this reserves slice.size in encoded, base64_decode then resizes a second std::string to the same size, and ColumnVarbinary finally copies the decoded bytes into its arena. Ordinary std::string storage does not run the Allocator<false> admission check, so one large external field can build roughly two input-sized buffers before the tracked column allocation rejects it; the new write path likewise materializes both value.to_string() and its Base64 output. Please decode/encode directly into checked destination storage (and avoid the hex path's equivalent whole-cell temporary), with a limited-memory scan/write test.
| while (type instanceof ArrayType) { | ||
| type = ((ArrayType) type).getItemType(); | ||
| } | ||
| if (type.isVarBinaryType()) { |
There was a problem hiding this comment.
[P2] Reject the resolved execution type rather than every original argument. For array_contains(ARRAY<STRING>, VARBINARY), the FOLLOW signature resolves the scalar to STRING and the existing implicit cast feeds the supported BE string kernel; array_position, count_equal, array_remove, and array_contains_all share that pattern. With default new type coercion, indexed-Any common-type selection likewise resolves mixed ARRAY<STRING>/ARRAY<VARBINARY> inputs to STRING for arrays_overlap, array_except, and array_union, whose BE implementations have ColumnString paths. This guard rejects all of them before coercion, and the new mixed-type test locks in the regression. Please reject signatures that resolve to VARBINARY, but allow binary inputs that resolve to supported execution types.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
) ### What problem does this PR solve? This is the first of five planned extractions from #67784, targeting `branch-4.1`. Binary `Field` values 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. - Own long binary Field values while keeping short values inline. Preserve execution type lengths and decoder bytes, fix binary literal encoding, and add Hive Base64 and hexadecimal decoding support. - Explicitly reject unsupported binary hash keys, IN, aggregates, predicates and computed partition transforms. Keep the existing FE comparison/group/join restrictions and existing binary scalar functions. Reject unsupported collection kernels before coercion. - Normalize fixed timezone offsets and preserve historical second offsets in both TIMESTAMPTZ formatting and parsing. Skip masked NULL payloads, reject unrepresentable local years, and preserve cast error/NULL behavior at boundaries. 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. Explicit `Asia/Shanghai` session 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_set` arities, 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 `validate` with 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) - Test - [x] Regression test (three self-checking suites added; execution pending CI) - [x] Unit Test - Behavior changed: - [x] Yes. Binary rejection and timestamp boundary behavior are described above. - Does this need documentation? - [x] No. This fixes existing type behavior without introducing a configuration option. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label ### 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. - Separate historical TIMESTAMPTZ wire-offset parsing from session fixed-zone limits in both parser paths. - Validate the complete UTC/GMT fixed offset and exclude rejected endpoint values from the timezone cache. - Decline FE string folding when the session-local year is outside the new BE display range. Preserve the CAST for BE evaluation in both cast modes instead of folding non-strict casts to NULL. - Validation: 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. - The corresponding master follow-up is in #68301. Master already has different timezone normalization and FE folding behavior; its additional TIMESTAMP_NS error-reporting fix does not apply to branch-4.1. ### CI test follow-up - Keep the binary literal test's owning Field alive while reading its StringView. Branch-4.1 now has the corresponding short/long embedded-NUL coverage using its execution API. - Replace the obsolete +15:00 rejection input with +24:00. Add generated historical-offset checks in both cast modes; all prior snapshot results are unchanged. - Validation: 34 focused ASAN BE tests passed on each branch. The lifetime error and the original SQL mismatch were reproduced. The complete cast regression suite passed in comparison mode from both branch checkouts against the reported master CI artifact. clang-format 16 passed; full clang-tidy remains blocked by pre-existing diagnostics. 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.
) ### What problem does this PR solve? This is the first of five planned extractions from #67784, targeting `branch-4.1`. Binary `Field` values 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. - Own long binary Field values while keeping short values inline. Preserve execution type lengths and decoder bytes, fix binary literal encoding, and add Hive Base64 and hexadecimal decoding support. - Explicitly reject unsupported binary hash keys, IN, aggregates, predicates and computed partition transforms. Keep the existing FE comparison/group/join restrictions and existing binary scalar functions. Reject unsupported collection kernels before coercion. - Normalize fixed timezone offsets and preserve historical second offsets in both TIMESTAMPTZ formatting and parsing. Skip masked NULL payloads, reject unrepresentable local years, and preserve cast error/NULL behavior at boundaries. 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. Explicit `Asia/Shanghai` session 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_set` arities, 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 `validate` with 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) - Test - [x] Regression test (three self-checking suites added; execution pending CI) - [x] Unit Test - Behavior changed: - [x] Yes. Binary rejection and timestamp boundary behavior are described above. - Does this need documentation? - [x] No. This fixes existing type behavior without introducing a configuration option. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label ### 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. - Separate historical TIMESTAMPTZ wire-offset parsing from session fixed-zone limits in both parser paths. - Validate the complete UTC/GMT fixed offset and exclude rejected endpoint values from the timezone cache. - Decline FE string folding when the session-local year is outside the new BE display range. Preserve the CAST for BE evaluation in both cast modes instead of folding non-strict casts to NULL. - Validation: 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. - The corresponding master follow-up is in #68301. Master already has different timezone normalization and FE folding behavior; its additional TIMESTAMP_NS error-reporting fix does not apply to branch-4.1. ### CI test follow-up - Keep the binary literal test's owning Field alive while reading its StringView. Branch-4.1 now has the corresponding short/long embedded-NUL coverage using its execution API. - Replace the obsolete +15:00 rejection input with +24:00. Add generated historical-offset checks in both cast modes; all prior snapshot results are unchanged. - Validation: 34 focused ASAN BE tests passed on each branch. The lifetime error and the original SQL mismatch were reproduced. The complete cast regression suite passed in comparison mode from both branch checkouts against the reported master CI artifact. clang-format 16 passed; full clang-tidy remains blocked by pre-existing diagnostics. 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.
…68396) ### What problem does this PR solve? Related PR: #68381. This is the master version of the second split from #67784, based on the primitives merged in #68301. Arrow batch conversion mixes protocol serialization with table-specific UUID handling, while writers construct schemas separately. Introduce explicit Doris, Python, Arrow Flight, Parquet, Hive, Iceberg and Paimon convertors with instance-owned schema parameters and timezone. Move schema construction/decoding into the convertors and route nested SerDe writes through the selected format. Separate Parquet, Hive and Iceberg writers and migrate existing callers. Preserve master's tracked Arrow memory pools, Iceberg statistics and timestamp-nanosecond support. Master does not yet contain the Paimon write backend or physical Variant table writes present on branch-4.1; this pick adds the converter interfaces without importing those features. Parquet timestamp encoding and external type mappings remain unchanged. Include the Python timezone regression correction from #68381: the single string output uses ARRAY<STRING>, so the lateral-view comparison reaches execution instead of failing on a STRUCT-versus-STRING comparison. Retain coverage for four session timezones, microseconds, pre-epoch values, NULLs, UDF, UDTF and UDAF. ### Release note Fix Python UDF timestamp conversion to preserve wall-clock values when the Arrow protocol declares a fixed-offset timezone. ### Check List (For Author) - Test - [x] Unit Test: explicit schemas and independent converter instances, nested/null values, UUID and fixed binary bytes, timestamp bindings, and Iceberg writer statistics. - [x] Regression test: Python UDF/UDTF/UDAF timezone comparisons and the existing timestamp snapshot corrections. - Behavior changed: - [x] Yes: align Python UDF conversion with its Arrow timezone declaration; reject invalid nested bindings before casts. - Does this need documentation? - [x] No. Validation: ASAN BE build and 310 focused tests passed (53 suites), covering Arrow conversion, Parquet/ORC, Variant SerDe and Python. All 42 affected C++ files passed clang-format 16, and header hygiene passed. The original UDTF declaration reproduced the SQL analysis error on an isolated FE; the corrected declaration passed the same analysis. Groovy and embedded Python checks passed. Full Python SQL and external-catalog regressions remain for CI. clang-tidy was attempted: the new converter's size warning was resolved; analysis remains blocked by a pre-existing unmatched NOLINTEND in core/types.h. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
5 participants
What problem does this PR solve?
This ports #68297 to
master, preserving the first of five planned extractions from #67784.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.
Master adaptation
VInPredicate::_prepare_zonemap_min_maxinterface in both the guard and its test.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: 18 FE tests passed with Checkstyle enabled, covering direct legality checks, nested/mixed/variadic VARBINARY arguments, both
collect_setarities, supported ordinary types, SQL analysis, and existing array rewrites. The new direct-legality tests reproduced missing rejection before the change.BE ASAN build and 199 tests passed across 17 suites using
run-be-ut.sh, including binary lifetime/SerDe/rejection, timestamp parsing/casts, and existing Arrow/Variant serialization coverage.VarBinaryUnsupportedCollectionTest: passed (13 unsupported expressions plus supported byte-preserving collection analysis). The FE test reactor and repository Checkstyle passed after cleaning stale branch build artifacts.Repository clang-format 16 check and build-header hygiene checks: passed; 31 changed C++ source/header files.
Groovy compilation of the three regression suites: passed. Live SQL regression execution remains pending CI.
clang-tidy was attempted but could not complete because master already contains an unmatched
NOLINTENDinbe/src/core/types.h. A diagnostic run with the compiler resource directory corrected reproduced that blocker; the other reported findings incolumn_varbinary.cppwere outside changed lines. This is not a clean clang-tidy result.The focused BE test source list and local test/build settings were 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, performance refactors, and test-style-only rewrites 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.