[core] Validate STRING ts field for merge_map_with_keytime at DDL - #10221
Open
LuciferYang wants to merge 2 commits into
Open
LuciferYang wants to merge 2 commits into
LuciferYang wants to merge 2 commits into
Conversation
The aggregate reads the ts field with getString and compares values lexicographically, but nothing checked the field type: a ROW whose ts field is TIMESTAMP (or INT, or anything non-string) is accepted and the raw bits are interpreted as a string — BinaryRow reads an unrelated offset/length as the value — so the wrong map entry is retained with no error. The documented contract says each key carries a string timestamp and the implementation reads this field as a string. Reject the schema in the factory when the resolved ts field (explicit or the default last field) is not STRING. Assisted-by: GLM-5.3
LuciferYang
marked this pull request as draft
September 27, 2026 03:04
LuciferYang
marked this pull request as ready for review
September 27, 2026 04:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The
merge_map_with_keytimeaggregator reads the per-key timestamp field withgetStringand compares values lexicographically, so a STRING ts field is the documented contract. The type was never validated, so a ROW whose ts field is TIMESTAMP, INT, or any non-string type was accepted, and at merge timegetStringreinterprets the raw bytes, silently retaining the wrong map entry on a key collision.This validates the ts field type at DDL only.
SchemaValidationnow resolves the ts field the same way the aggregator does (the explicitly configuredfields.<f>.ts-field, else the default last field of the map value ROW) and requires it to be VARCHAR or CHAR, throwingIllegalArgumentExceptionnaming the field. The check runs at CREATE/ALTER validation, not when the merge function is built, so a bad new schema is rejected up front while an already-created table is still readable (its rows can be read for migration rather than the table failing to open). The factory no longer performs a build-time type check.Tests
SchemaValidationTest.testMergeMapWithKeyTimeRejectsNonStringTsFieldAtDdlpins that DDL validation rejects amerge_map_with_keytimeschema whose ts field is TIMESTAMP, and whose explicitly configuredts-fieldpoints at an INT column, while a STRING ts field is accepted.FieldAggregatorTest.testFieldMergeMapWithKeyTimeAggBuildIgnoresTsFieldTypepins that building the aggregator no longer throws for a non-string ts field, so opening an existing table does not fail.API and Format
No.
Documentation
No.