Repository navigation
[python] Support UpdateColumnDefaultValue in SchemaManager - #10675
Stephen0421 wants to merge 1 commit into
Conversation
JingsongLi
left a comment
There was a problem hiding this comment.
The setter has useful cross-engine behavior, but several defaults pass the new Python validator and are persisted successfully, then prevent the existing Java FileStoreTable writer from initializing. Each case below was verified by loading the exact Python-written schema and calling table.newWrite().
There is also a mismatch in the opposite direction: Python rejects TIMESTAMP 2024-01-01 24:00:00, whereas the Java SMART formatter accepts it as the following midnight. Please include acceptance and rejection cases in cross-runtime parity tests.
Validation: 26 Python schema tests passed. Four targeted Flink integration checks also passed: ordinary Python defaults 5 → 7 → None affected future inserts while preserving existing rows, and each of the three accepted-invalid defaults above caused the expected Flink INSERT initialization failure. All final Maven checks were enabled.
| _check_java_int(body[:colon1]) | ||
| colon2 = body.find(":", colon1 + 1) | ||
| if colon2 < 0: | ||
| _check_java_int(body[colon1 + 1:]) |
There was a problem hiding this comment.
[P2] Reject TIME forms that the Java reader/writer cannot cast
For a TIME column, UpdateColumnDefaultValue(["b"], "12:30+01:00") currently commits successfully, but loading that persisted schema in Java and calling table.newWrite() throws DateTimeException. This parser finds the separators only after removing the offset; the current Java DateTimeUtils.parseTime examines the original string, so this specific form is rejected there (12:30:00+01:00 and 12:30+01 are accepted by both). The new validation must match the deployed Java cast before publishing the default, otherwise a successful Python ALTER leaves JVM writers unable to start.
There was a problem hiding this comment.
Fixed. _parse_time now follows DateTimeUtils.parseTime index by index: colon1, colon2 and dot are searched in the original string, offset included, and substrings are bounded the way String.substring is. 12:30+01:00 and 12+01:00 are now rejected, while 12:30:00+01:00, 12:30+01 and 12+01 are still accepted. These cases are in the unit tests, and 12:30+01:00 is also in the interop rejection list.
| # seconds. appendFraction has a minimum width of 0, so a bare '.' is allowed. | ||
| # Anchor with \Z, not $, so a trailing newline does not slip through. | ||
| _TIMESTAMP_SPACE_RE = re.compile( | ||
| r"^(\d{4})-(\d{1,2})-(\d{1,2})" |
There was a problem hiding this comment.
[P2] Use the JVM timestamp formatter digit grammar
Python \d includes Unicode digits, so a TIMESTAMP(3) default such as 2024-01-01 00:00:00 is accepted and persisted. The Java formatter rejects it at index 0 with DateTimeParseException; the writer then fails during default initialization. This is reproducible from the exact Python-written schema and also affects TIMESTAMP_LTZ. Restrict these numeric components to the characters accepted by Java and add a persisted-schema interoperability test.
There was a problem hiding this comment.
Fixed. The timestamp regexes use [0-9] instead of \d, so full-width and other Unicode digits in any component (date, time or fraction) are rejected for both TIMESTAMP and TIMESTAMP_LTZ. The year now follows yyyy exactly: four digits, or + followed by 5 to 19 digits within the era-year range. The persisted-schema interop test covers your example for both TIMESTAMP(3) and TIMESTAMP_LTZ(3).
| match = _DECIMAL_RE.fullmatch(text) | ||
| if match is None: | ||
| raise ValueError("Invalid decimal {}".format(text)) | ||
| Decimal(text) |
There was a problem hiding this comment.
[P2] Enforce Java BigDecimal exponent and scale limits
Decimal(text) accepts 1e2147483648 (and 1e-2147483648), so the new setter persists it for DECIMAL(10,2). Java BigDecimal rejects these with NumberFormatException because its exponent/scale range is bounded, and initializing the writer for that table subsequently fails. This is distinct from ordinary precision overflow, which Java may cast to null. Validate Java-compatible exponent/scale bounds before committing the schema.
There was a problem hiding this comment.
Fixed. Decimal(text) is gone, and validation now checks both places where Java fails:
new BigDecimal(String): the exponent and the resulting scale (fraction digits minus exponent) must fit in an int, otherwiseNumberFormatException. This covers1e2147483648,1e-2147483648and1.5e-2147483647.Decimal.fromBigDecimalcallssetScale(columnScale). For a non-zero value, that needsBigInteger.TEN.pow(raise), which throwsArithmeticExceptiononceraise >= 536870920. That's the point wherebitLength * raise / 32exceedsMAX_MAG_LENGTH. This covers1e2147483647and1e-2147483646. Zero values never build that power, so0e-2147483645stays valid, as in Java.
Ordinary precision overflow such as 12345678901 or 1e100000 on DECIMAL(10, 2) is still accepted and becomes a null default, matching Java.
9f4b9a6 to
177cadc
Compare
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS.
The earlier TIME-offset, Unicode timestamp, BigDecimal exponent/scale, and SMART-midnight concerns have regression coverage and are fixed in this head. Two additional accepted-default cases still persist schemas that Java can load but cannot write, detailed inline.
Both findings were verified by applying the exact Python schema change, reopening the same on-disk schema in Java, and constructing the actual table writer. Existing tables without the default work; valid calendar/quoted defaults and clearing the default restore successful writer initialization. The main reviewer independently reran both with this head's compiled Core/API classes.
Verification: 26 Python schema tests and the new Python interoperability fixture passed; the added Java interoperability method also passed under an external JUnit adapter pointing at that exact fixture. Normal JDK 8 Maven verify with DefaultValueRowTest and changed-file flake8 passed. The first Maven interoperability invocation looked in the default in-checkout fixture directory and found no table; the external adapter corrected that test setup without changing repository files. Independent cast-parity probes examined 25,103 cases. Remote CI is red; inspected failures include Maven Central HTTP502 and the unrelated mixed shard-updater API case.
| return | ||
| if root == "TIMESTAMP": | ||
| if _JAVA_NUMERIC_RE.fullmatch(text): | ||
| _parse_integral(text, "BIGINT") |
There was a problem hiding this comment.
[P2] Check TIMESTAMP precision before accepting an epoch default
For a valid TIMESTAMP(2) column, UpdateColumnDefaultValue(["ts"], "1700000000000") passes this BIGINT-only check and commits schema-1, but reopening the same schema and calling newBatchWriteBuilder().newWrite() in Java throws "Unsupported precision: 2" from BinaryStringUtils.fromMillisToTimestamp. That conversion only supports precisions 0, 3, 6 and 9; the table works with no default, with a calendar-string default, or after clearing it. Inspect the declared timestamp precision and reject unsupported epoch/precision combinations before committing, with a persisted Python-to-Java writer regression.
There was a problem hiding this comment.
Thanks for catching this! Fixed in 6ff715e.
For TIMESTAMP without time zone, a numeric default goes through BinaryStringUtils.fromMillisToTimestamp, which only supports precision 0, 3, 6 and 9. The validator now runs the same checks in the same order as Java:
- The value must match
StringUtils.isNumericand fit in a BIGINT. Otherwise it is rejected as overflow. - The column precision must be 0, 3, 6 or 9. Otherwise it is rejected, matching
Unsupported precision: pin Java.
Notes:
- A bare
TIMESTAMPuses the default precision 6. ANOT NULLsuffix does not affect how the precision is read. - Only the numeric path is limited. A string timestamp such as
TIMESTAMP(2)with'2024-01-01 12:00:00.12'is still accepted, because that path has no precision check in Java. TIMESTAMP WITH LOCAL TIME ZONEhas no numeric path in Java, so numeric defaults for it are still rejected.
Tests:
test_rejects_scalar_defaults_java_rejects:TIMESTAMP(2)with1700000000000,TIMESTAMP(1)with-1, andTIMESTAMP(7)with0.test_accepts_scalar_defaults_java_accepts:TIMESTAMP(0),TIMESTAMP(9), bareTIMESTAMPandTIMESTAMP(3) NOT NULLwith1700000000000, plusTIMESTAMP(2)with a string timestamp.test_invalid_defaults_do_not_commit_schema: committingTIMESTAMP(2)with1700000000000fails and leaves the schema id unchanged.- Python-to-Java interop: Python sets a
TIMESTAMP(3)default of1700000000000.JavaPyE2ETest#testReadPythonColumnDefaultValuesthen opensnewWrite(), fills the default and reads back2023-11-14T22:13:20.
|
|
||
|
|
||
| def _unquote(value: str) -> str: | ||
| if len(value) >= 2 and value.startswith("'") and value.endswith("'"): |
There was a problem hiding this comment.
[P2] Reject a lone quote before persisting a string default
A one-character apostrophe passes this length guard unchanged and is accepted as a STRING default. UpdateColumnDefaultValue(["name"], "'") therefore commits successfully, but opening a Java writer on that persisted schema throws StringIndexOutOfBoundsException in DefaultValueUtils.convertDefaultValue: Java treats the value as quoted and attempts substring(1, 0). Actual quoted "hello" and a correctly wrapped apostrophe (three quote characters) both initialize the writer successfully. Reject the lone-quote form before publishing the schema, rather than accepting a value the JVM cannot cast, and add an alteration followed by Java writer initialization test.
There was a problem hiding this comment.
Good catch, thanks! Fixed in 6ff715e.
Before casting, DefaultValueUtils.convertDefaultValue strips surrounding quotes for every type:
if (defaultValueStr.startsWith("'") && defaultValueStr.endsWith("'")) {
defaultValueStr = defaultValueStr.substring(1, defaultValueStr.length() - 1);
}For a lone ', this calls substring(1, 0) and throws StringIndexOutOfBoundsException. Python's _unquote now rejects exactly "'" for all types. Previously only STRING, CHAR and BINARY could let it through; numeric and temporal types already failed to parse it.
Other quote forms behave as before, matching Java:
''becomes an empty string, which is a valid STRING default.'abc(quote only at the start) is kept as is.
Tests:
test_rejects_scalar_defaults_java_rejects:STRING,CHAR(1)andBYTESwith'.test_accepts_scalar_defaults_java_accepts:STRINGwith''and'abc.test_invalid_defaults_do_not_commit_schema: committingSTRINGwith'fails and leaves the schema id unchanged.- Python-to-Java interop: Python checks that
'is rejected, then sets a STRING default of''.JavaPyE2ETest#testReadPythonColumnDefaultValuesreads it back as an empty string afternewWrite().
Apply UpdateColumnDefaultValue when committing schema changes, so a column default can be set, changed and cleared through the Python SchemaManager. Validate the value against the target type before storing it, mirroring the Java DefaultValueUtils string casts for booleans, numbers, dates, times and timestamps. Widen new_default_value to Optional[str], matching the @nullable contract on the Java side, where None clears the default. ARRAY, MAP and ROW defaults are accepted by Java but are not parsed here yet, so they are rejected with an explicit message.
177cadc to
6ff715e
Compare
Purpose
SchemaManager.commit_changessilently ignoredUpdateColumnDefaultValue: thechange was accepted but never applied, so a column default could not be set,
changed or cleared from Python. This PR applies it and validates the value
against the target type before storing it.
DefaultValueUtilsstring casts for booleans,numbers, dates, times and timestamps. The stored value is still the original
string; only castability is checked.
UpdateColumnDefaultValue.new_default_valueis widened fromstrtoOptional[str], matching the@Nullablecontract ofDefaultValueUtils#validateDefaultValueon the Java side, whereNoneclears the default.
SchemaChange.update_column_default_valueis widened tomatch.
_update_nested_columnpath, the same way
UpdateColumnCommentworks.committed.
Known gap: Java also accepts
ARRAY,MAPandROWdefaults viaStringToArrayCastRule,StringToMapCastRuleandStringToRowCastRule.Python does not parse those forms yet and rejects them with an explicit
message rather than accepting a value it cannot validate.
Tests
pypaimon/tests/schema_manager_test.pyaddsTestUpdateColumnDefaultValue:column; assert the schema id does not advance in either case.
ARRAYdefault with the explicit unsupported-type message.Every case was checked against a compiled
DefaultValueUtilsbefore beingadded, covering the boundaries that are easy to get wrong:
INTaccepting".5"and"10.",DOUBLEaccepting"0x1.0p0"and" 1.5 "but not"1_000", negative epochs forDATE/TIME/TIMESTAMP,TIMEnotbounding its components, the timestamp formatter's SMART resolver clamping
day-of-month, and
TIMESTAMPaccepting an epoch whileTIMESTAMP_LTZdoesnot.