laskoviymishka opened a new pull request, #1674:
URL: https://github.com/apache/iceberg-go/pull/1674
`checkForRedundantPartitions` used `PartitionSpec.sourceIdToFields`, but
that map is only populated by `initialize()`. `NewPartitionSpecOpts` calls
`initialize()` after all options have been applied, so during construction the
map is nil and the check never runs. As a result, the builder accepted multiple
partition fields with the same transform on the same source column.
`UnmarshalJSON` rejects the same spec through `validatePartitionFields`,
which meant the builder could produce metadata that the library could not read
back:
```text
build: err=<nil> nfields=2
marshal:
{"spec-id":1,"fields":[{"source-id":1,...,"transform":"bucket[16]"},
{"source-id":1,...,"transform":"bucket[16]"}]}
read back own output: err=invalid partition spec: redundant partition field
for source IDs [1] and transform bucket[16]
```
This change validates the final field set in `NewPartitionSpecOpts` using
`validatePartitionFields` instead of trying to catch duplicates as each field
is added. That gives the constructor and parser a single definition of a
redundant partition field. `BindToSchema` also goes through this constructor,
so it gets the same validation.
`NewPartitionSpec` and `NewPartitionSpecID` remain unchanged because neither
can return an error. Their doc comments now note that they may produce specs
that do not survive a metadata round trip.
I also changed `validatePartitionFields` to compare transforms with
`Transform.Equals` rather than using a string key based on `fmt.Sprintf`. This
behaves the same for the eight transforms currently implemented, but avoids
maintaining a separate definition of transform identity. Since this calls a
method on the transform, nil transforms are now rejected explicitly instead of
causing a dereference.
Repeated `void` fields remain allowed. `void` is used as a tombstone for
dropped partition fields, and a v1 spec may legitimately contain several
dropped fields for the same source column. `BindToSchema` reconstructs those
specs through the builder, so rejecting repeated `void` fields would break that
path. A test covers this case.
One intentional difference from Java remains. Java's `dedupName` treats
year, month, day, and hour as one partition name per source column, so it
rejects `year(ts)` together with `month(ts)`. Go currently accepts that
combination. This PR does not change that behavior; the validator has a TODO,
and a test records the current result.
This does make `NewPartitionSpecOpts` stricter. Callers that currently build
redundant specs will start receiving an error. That is observable, but those
specs could not be read back by the library anyway.
The review also uncovered a separate existing bug that is not included here:
a source-less `void` tombstone fails `BindToSchema` with `cannot find source
column with id: 0 in schema`. The behavior is the same on `main` and this
branch. `UnmarshalJSON` accepts that shape intentionally, so `BindToSchema`
should be able to replay it. I can file that separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
