Skip to content

[core] Support more timestamp types in merge_map_with_keytime - #11031

Open
PyRSA wants to merge 8 commits into
apache:masterfrom
PyRSA:feature/merge-map-with-keytime-more-types
Open

PyRSA wants to merge 8 commits into
apache:masterfrom
PyRSA:feature/merge-map-with-keytime-more-types

Conversation

@PyRSA

@PyRSA PyRSA commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Purpose

This PR extends the supported timestamp field types in merge_map_with_keytime.

The following types are supported:

  • CHAR / VARCHAR
  • TINYINT / SMALLINT / INTEGER / BIGINT
  • DATE / TIME
  • DECIMAL
  • TIMESTAMP
  • TIMESTAMP_LTZ

Character timestamp fields retain Java String.compareTo ordering semantics. The Python implementation is also aligned with the Java behavior.

Tests

  • Add Java tests for supported timestamp types and comparison behavior.
  • Add Python tests for supported timestamp types and comparison behavior.
  • Verify that newer timestamps replace existing values, while equal or older timestamps do not.
  • git diff --check

@Akash3121 Akash3121 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The typed timestamp extension is useful for epoch and timestamp fields. The head passed 119 Java tests and 167 Python tests. It conflicts with master after #11043 in the Java aggregator/factory/tests and the Python aggregator. Please preserve merge-map.overwrite-on-equal-ts for both string and typed comparisons when rebasing.

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A second interoperability check for the newly supported timestamp precision.

@PyRSA
PyRSA force-pushed the feature/merge-map-with-keytime-more-types branch 7 times, most recently from 860ec82 to 77d3fa4 Compare November 11, 2026 13:31

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requirement fit: SUPPORTED. Implementation: FINDINGS. DECIMAL extends the Java types supported after #11084, and the public Python nanosecond-read fix has real correctness value. The previous DOUBLE and TIMESTAMP(9)/TIMESTAMP_LTZ(9) examples now pass, and overwrite-on-equal behavior is preserved.

Reviewed all eleven files, generic row conversion, Java/Python comparison and native routing. 128 Java field-aggregator tests and 194 Python tests passed. Two additional Java table probes passed normal verify: DECIMAL precision10/25, actual writes, reopened reads, equal-timestamp policy and compaction. Additional real filesystem probes reproduce two remaining compatibility/interop problems below: three exact-baseline legacy controls succeed but current readers fail, and native/Python readers choose different string-timestamp winners. The native probe uses Python planning and omits only scheduling kwargs unsupported by the installed Rust0.4 wheel; Python read fallback is forbidden. Current public Rust main independently still uses the same UTF-8 string comparison. Project flake8 and diff whitespace checks passed.


ts_type = row_type.fields[ts_field_index].type
ts_base = _atomic_base_name(ts_type)
if ts_base not in _KEYTIME_TIMESTAMP_TYPES:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve the remaining historically readable timestamp types

The FLOAT/DOUBLE exception does not fully address the earlier persisted-table compatibility issue. The old Python aggregator also accepted ordered BINARY/VARBINARY and BOOLEAN fields; this whitelist rejects them before reading existing files. I independently reproduced all three with filesystem PK aggregation tables and two committed versions. For BINARY(4)/VARBINARY the timestamps are ordinary big-endian epoch bytes 9 and 10: the exact merge-base aggregator reads the newer entry, while this head reopens the identical files and raises ValueError. BOOLEAN false/true has the same regression. Preserve historical comparison for these types too, or handle a compatibility migration separately; an extension should not make previously readable tables inaccessible.

return False
ts_type = row_type.fields[ts_index].type
# None => type parse / non-atomic failure; not a supported native type.
if _atomic_type_base_name(ts_type) not in _NATIVE_KEYTIME_STRING_TYPES:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Require matching string comparison before enabling native reads

This gate allows STRING timestamps, but the new Java/Python comparison is UTF-16 while Rust still compares &str in UTF-8 order. With two persisted versions of the same map key, first at U+E000 and then U+10000, this head's Python reader keeps the first value and its permitted native reader keeps the second. Both ASCII controls pass, and native fallback was forbidden, so the same table's result depends on the execution backend. The current public Rust merge_map.rs still contains this comparison (blob ff388b217ef86d9420ac35dbda1d6a5088121dba). Fall back for this aggregator until the native comparator matches, or update/gate native support together with the semantics change.

PyRSA and others added 8 commits November 13, 2026 22:54
…lback

Inline the STRING timestamp probe so NativeTableRead stays within the
execution.native boundary and passes the backend isolation check.
Route all merge_map_with_keytime reads through Python before Native
opens (STRING UTF-16 vs Rust UTF-8; typed keytime never had a correct
Native path). Drop the Python timestamp whitelist so historical
BINARY/VARBINARY/BOOLEAN and other comparable types remain readable.
@PyRSA
PyRSA force-pushed the feature/merge-map-with-keytime-more-types branch from 6e5cd5b to 472f557 Compare November 13, 2026 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants