[CORE-8532] Disallow certain schema evolution actions if a field appears in the partition spec #25114
Conversation
This file contains 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
oleiman
changed the title
CI test resultstest results on build#62039
test results on build#62052
test results on build#62068
test results on build#62129
test results on build#62149
test results on build#62177
|
oleiman
force-pushed
the
dlib/core-8532/promotion-allowability
branch
from
01658cd to
202a811
Compare
oleiman
force-pushed
the
dlib/core-8532/promotion-allowability
branch
from
202a811 to
5d22c2d
Compare
oleiman
force-pushed
the
dlib/core-8532/promotion-allowability
branch
5 times, most recently
from
472426e to
523305a
Compare
|
/ci-repeat 1 |
|
CI Failures:
|
andrwng
reviewed
Feb 21, 2025
| @@ -540,6 +547,7 @@ static const std::vector<struct_evolution_test_case> valid_cases{ | |||
| && dst_nested.fields.empty() | |||
| && removed(*src_nested.fields.back()); | |||
| }, | |||
| .pspec = "(nested)", // TODO(oren): seems wrong frankly, should this fail? | |||
There was a problem hiding this comment.
The source columns, selected by ids, must be a primitive type and cannot be contained in a map or list, but may be nested in a struct.
Hmm yea this is interesting. Maybe not in this PR, but feels like we should treat it like we would a missing field when resolving
redpanda/src/v/iceberg/partition.cc
Lines 38 to 40 in 5fe9fc9
There was a problem hiding this comment.
ohh, i see. so a struct or map or whatever isn't even a valid source column? I don't think partition spec resolution is detecting that at all then. See L448 in this file e.g.... seems like a bug.
| query_engine=QUERY_ENGINES, | ||
| use_partition_spec=[True, False], | ||
| ) | ||
| def test_partition_spec_evo(self, cloud_storage_type, query_engine, |
There was a problem hiding this comment.
Not related to this PR, but it's worth trying these out with a a real catalog instead of just filesystem_catalog
oleiman
force-pushed
the
dlib/core-8532/promotion-allowability
branch
2 times, most recently
from
5961ed2 to
fbb780d
Compare
Retry command for Build#62129please wait until all jobs are finished before running the slash command |
Logical DATE type should be bound to AVRO_INT only. Previously we used
AVRO_LONG, so the Avro lib would throw when exercising this code path.
Also adds a unit test for Iceberg types that are backed by Avro logicalTypes.
Note that timestamptz serialization is currently bugged, as it relies on an
extra non-standard field ('adjust-to-utc') in the Avro type description.
Technically this should appear in both micro precision timestamp types,
'false' for 'timestamp'; 'true' for 'timestamptz'. As written, both timestamp
types serialize to logical type timestamp-micros, with no way to distinguish
between the two in deser.
See https://iceberg.apache.org/spec/#avro for detail
Signed-off-by: Oren Leiman <[email protected]>
To support special promotions that affect partition transforms type_promoted: - no - yes - unless_partition Signed-off-by: Oren Leiman <[email protected]>
Signed-off-by: Oren Leiman <[email protected]>
Specify either an annotate error or a validate error. This simplifies some of the conditional logic around instantiating test suites. No functional changes. Signed-off-by: Oren Leiman <[email protected]>
Signed-off-by: Oren Leiman <[email protected]>
Signed-off-by: Oren Leiman <[email protected]>
…otion Signed-off-by: Oren Leiman <[email protected]>
Though this restriction does not appear in the spec, dropping a data column that also appears in the table's partition spec can cause validation errors in clients, downstream. It may be possible to avoid this by performing live partition spec reconciliation inline with the schema update, but for now we simply reject such an update out of hand. Also slightly refactors to consolidate error checks in validate_schema_transform. Signed-off-by: Oren Leiman <[email protected]>
Signed-off-by: Oren Leiman <[email protected]>
oleiman
force-pushed
the
dlib/core-8532/promotion-allowability
branch
from
fbb780d to
035c4e3
Compare
|
force push typos |
rockwotj
previously approved these changes
Feb 23, 2025
|
/ci-repeat 1 |
7 tasks
andrwng
approved these changes
Feb 24, 2025
|
/ci-repeat 1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
General idea is as follows (quoted from the spec):
In general, the type promotion rules preclude this, but date -> timestamp promotions are called out specifically as potentially rule violating, if the promoted field appears in the partition spec. So this diff prevents that by wiring the current partition spec into the compat module and adding a check to the validation path.
This PR also disallows removal of any data field which also appears in the current partition spec at the time of removal. This is technically outside the spec and needs followup ecosystem investigation, but it appears that dropping a column in this way can cause downstream catalog queries to fail. The safest thing at this stage is to simply disable the capability for now.
This PR also fixes a bug in
iceberg/schema_avrowhereby we would try to serializedate_typeto anAVRO_LONGwith logical type "date", which itself is restricted toAVRO_INT. Fix includes a unit test.Backports Required
Release Notes