You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/04 23:34:45 UTC
[PR] fix(conflict_validation): reject invalid isolation-level values [iceberg-go]
fallintoplace opened a new pull request, #1398:
URL: https://github.com/apache/iceberg-go/pull/1398
## Summary
- reject malformed table conflict isolation-level properties at validation time
- propagate errors from overwrite/row-delta conflict checks
- add regression tests for helper and validator error behavior
## Testing
- go test ./table -count=1
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(conflict_validation): reject invalid isolation-level values [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on code in PR #1398:
URL: https://github.com/apache/iceberg-go/pull/1398#discussion_r3525748404
##########
table/conflict_validation.go:
##########
@@ -96,19 +96,23 @@ const (
WriteUpdateIsolationLevelDefault = IsolationSerializable
)
+// ErrInvalidIsolationLevel is returned when a configured isolation level
+// value is invalid.
+var ErrInvalidIsolationLevel = errors.New("invalid isolation level")
+
// readIsolationLevel returns the isolation level for the given
-// property key, falling back to defVal for a missing or unrecognized
-// value.
-func readIsolationLevel(props iceberg.Properties, key string, defVal IsolationLevel) IsolationLevel {
+// property key, falling back to defVal when the key is absent.
+func readIsolationLevel(props iceberg.Properties, key string, defVal IsolationLevel) (IsolationLevel, error) {
v, ok := props[key]
if !ok {
- return defVal
+ return defVal, nil
}
+
switch IsolationLevel(v) {
case IsolationSerializable, IsolationSnapshot:
- return IsolationLevel(v)
+ return IsolationLevel(v), nil
default:
- return defVal
+ return defVal, fmt.Errorf("%w: %q for property %q", ErrInvalidIsolationLevel, v, key)
Review Comment:
This rejects anything that isn't exactly lowercase "serializable"/"snapshot" (empty string included). I think Java's fromName is case-insensitive — if so, a config ported from Java/Spark with "SERIALIZABLE" would now fail here. Worth confirming; lowercasing v before the switch would keep parity.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(conflict_validation): reject invalid isolation-level values [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1398:
URL: https://github.com/apache/iceberg-go/pull/1398#discussion_r3536301154
##########
table/snapshot_producers.go:
##########
@@ -259,7 +259,11 @@ func (of *overwriteFiles) validate(cc *conflictContext) error {
if of.base.op == OpDelete {
key, defVal = WriteDeleteIsolationLevelKey, WriteDeleteIsolationLevelDefault
}
- if readIsolationLevel(of.base.txn.meta.props, key, defVal) != IsolationSerializable {
+ level, err := readIsolationLevel(of.base.txn.meta.props, key, defVal)
Review Comment:
this validation only runs once we actually reach `validate()` — and on the first commit attempt `cc` is nil and we bail at the top before ever getting here, so an invalid isolation-level silently commits on the fast path.
the PR title says we reject invalid values, but really we reject them only on certain paths. i'd lift the isolation-level check somewhere unconditional (top of `validate()`, or at commit time before the producer runs) so it fires regardless of file composition or retry state.
##########
table/conflict_validation.go:
##########
@@ -96,19 +97,23 @@ const (
WriteUpdateIsolationLevelDefault = IsolationSerializable
)
+// ErrInvalidIsolationLevel is returned when a configured isolation level
+// value is invalid.
+var ErrInvalidIsolationLevel = errors.New("invalid isolation level")
+
// readIsolationLevel returns the isolation level for the given
-// property key, falling back to defVal for a missing or unrecognized
-// value.
-func readIsolationLevel(props iceberg.Properties, key string, defVal IsolationLevel) IsolationLevel {
+// property key, falling back to defVal when the key is absent.
+func readIsolationLevel(props iceberg.Properties, key string, defVal IsolationLevel) (IsolationLevel, error) {
v, ok := props[key]
if !ok {
- return defVal
+ return defVal, nil
}
- switch IsolationLevel(v) {
+
+ switch IsolationLevel(strings.ToLower(v)) {
case IsolationSerializable, IsolationSnapshot:
- return IsolationLevel(v)
+ return IsolationLevel(strings.ToLower(v)), nil
default:
Review Comment:
the empty-string case is the one I'd push back on here. right now `''` (key present, empty value) becomes a hard, non-retryable commit failure — but that's a value some catalogs write when you "clear" a property without deleting the key, and some engines use it as a null/default repr.
the spec only says a missing key falls back to default; it's silent on empty string. i'd keep `''` on the fallback path alongside the absent-key case and only error on non-empty unrecognized values. that also means dropping the new "empty string returns error" test. wdyt?
##########
table/conflict_validation.go:
##########
@@ -96,19 +97,23 @@ const (
WriteUpdateIsolationLevelDefault = IsolationSerializable
)
+// ErrInvalidIsolationLevel is returned when a configured isolation level
+// value is invalid.
Review Comment:
could we add a line noting this is deliberately terminal — it doesn't wrap `ErrCommitFailed`, so it exits `doCommit` without retry rather than being treated as a retryable conflict?
every other exported sentinel here documents its retry semantics, and without the note it's easy for the next person to "fix" this by wrapping `ErrCommitFailed` and quietly make a config error retryable.
##########
table/conflict_validation.go:
##########
@@ -96,19 +97,23 @@ const (
WriteUpdateIsolationLevelDefault = IsolationSerializable
)
+// ErrInvalidIsolationLevel is returned when a configured isolation level
+// value is invalid.
+var ErrInvalidIsolationLevel = errors.New("invalid isolation level")
+
// readIsolationLevel returns the isolation level for the given
-// property key, falling back to defVal for a missing or unrecognized
-// value.
-func readIsolationLevel(props iceberg.Properties, key string, defVal IsolationLevel) IsolationLevel {
+// property key, falling back to defVal when the key is absent.
+func readIsolationLevel(props iceberg.Properties, key string, defVal IsolationLevel) (IsolationLevel, error) {
v, ok := props[key]
if !ok {
- return defVal
+ return defVal, nil
}
- switch IsolationLevel(v) {
+
+ switch IsolationLevel(strings.ToLower(v)) {
case IsolationSerializable, IsolationSnapshot:
Review Comment:
minor: we lower-case `v` twice on the match path — once for the switch discriminant, once in the returned value. worth computing it once:
```go
lower := IsolationLevel(strings.ToLower(v))
switch lower {
case IsolationSerializable, IsolationSnapshot:
return lower, nil
default:
return defVal, fmt.Errorf("%w: %q for property %q", ErrInvalidIsolationLevel, v, key)
}
```
keeping the original `v` in the error message is the right call — nice for a faithful repro.
##########
table/row_delta.go:
##########
@@ -230,8 +230,11 @@ func (rd *RowDelta) validate(cc *conflictContext) error {
}
if len(eqDeleteFiles) > 0 {
- level := readIsolationLevel(rd.txn.meta.props,
+ level, err := readIsolationLevel(rd.txn.meta.props,
Review Comment:
same gating issue on this path — the read + validate lives inside `if len(eqDeleteFiles) > 0`, so a RowDelta carrying only positional deletes with an invalid `write.delete.isolation-level` commits without error. pulling the property validation up into a single unconditional step covers this case too.
##########
table/producer_validate_test.go:
##########
@@ -165,18 +165,34 @@ func TestOverwriteFiles_IsolationKeySplitByOp(t *testing.T) {
WriteUpdateIsolationLevelKey: string(IsolationSerializable),
}
+ deleteLevel, err := readIsolationLevel(props, WriteDeleteIsolationLevelKey, WriteDeleteIsolationLevelDefault)
+ require.NoError(t, err)
// OpDelete must resolve to the delete key (snapshot).
assert.Equal(t, IsolationSnapshot,
- readIsolationLevel(props, WriteDeleteIsolationLevelKey, WriteDeleteIsolationLevelDefault),
+ deleteLevel,
"OpDelete path must consult write.delete.isolation-level")
// Every other op (OpOverwrite, OpReplace) must resolve to the
// update key (serializable).
+ updateLevel, err := readIsolationLevel(props, WriteUpdateIsolationLevelKey, WriteUpdateIsolationLevelDefault)
+ require.NoError(t, err)
assert.Equal(t, IsolationSerializable,
- readIsolationLevel(props, WriteUpdateIsolationLevelKey, WriteUpdateIsolationLevelDefault),
+ updateLevel,
"non-delete ops must consult write.update.isolation-level")
}
+// TestOverwriteFiles_ValidateInvalidIsolationLevel verifies that malformed
+// isolation settings fail the validator before conflict scanning runs.
+func TestOverwriteFiles_ValidateInvalidIsolationLevel(t *testing.T) {
+ cc := newEmptyConflictContext(t)
+ txn := newValidateTestTxn(t, iceberg.Properties{
+ WriteUpdateIsolationLevelKey: "not-a-level",
+ })
Review Comment:
this only exercises the non-delete branch (update key, `op != OpDelete`). the `op == OpDelete` branch that swaps in `WriteDeleteIsolationLevelKey` never gets an invalid-value test, so a transposition of the two keys wouldn't be caught. worth a sibling case with `of.base.op = OpDelete` and an invalid value under the delete key.
##########
table/conflict_validation_test.go:
##########
@@ -28,52 +28,73 @@ import (
func TestReadIsolationLevel(t *testing.T) {
tests := []struct {
- name string
- props iceberg.Properties
- key string
- defVal IsolationLevel
- want IsolationLevel
+ name string
+ props iceberg.Properties
+ key string
+ defVal IsolationLevel
+ want IsolationLevel
+ wantErr bool
}{
{
- name: "missing key falls back to default",
- props: iceberg.Properties{},
- key: WriteDeleteIsolationLevelKey,
- defVal: IsolationSerializable,
- want: IsolationSerializable,
+ name: "missing key falls back to default",
+ props: iceberg.Properties{},
+ key: WriteDeleteIsolationLevelKey,
+ defVal: IsolationSerializable,
+ want: IsolationSerializable,
+ wantErr: false,
},
{
- name: "explicit serializable",
- props: iceberg.Properties{WriteDeleteIsolationLevelKey: "serializable"},
- key: WriteDeleteIsolationLevelKey,
- defVal: IsolationSnapshot,
- want: IsolationSerializable,
+ name: "explicit serializable",
+ props: iceberg.Properties{WriteDeleteIsolationLevelKey: "serializable"},
+ key: WriteDeleteIsolationLevelKey,
+ defVal: IsolationSnapshot,
+ want: IsolationSerializable,
+ wantErr: false,
},
{
- name: "explicit snapshot",
- props: iceberg.Properties{WriteDeleteIsolationLevelKey: "snapshot"},
- key: WriteDeleteIsolationLevelKey,
- defVal: IsolationSerializable,
- want: IsolationSnapshot,
+ name: "explicit snapshot",
+ props: iceberg.Properties{WriteDeleteIsolationLevelKey: "snapshot"},
+ key: WriteDeleteIsolationLevelKey,
+ defVal: IsolationSerializable,
+ want: IsolationSnapshot,
+ wantErr: false,
},
{
- name: "unrecognized value falls back to default",
- props: iceberg.Properties{WriteDeleteIsolationLevelKey: "repeatable-read"},
- key: WriteDeleteIsolationLevelKey,
- defVal: IsolationSerializable,
- want: IsolationSerializable,
+ name: "explicit snapshot (mixed case)",
+ props: iceberg.Properties{WriteDeleteIsolationLevelKey: "Snapshot"},
+ key: WriteDeleteIsolationLevelKey,
+ defVal: IsolationSerializable,
+ want: IsolationSnapshot,
+ wantErr: false,
},
{
- name: "empty string falls back to default",
- props: iceberg.Properties{WriteDeleteIsolationLevelKey: ""},
- key: WriteDeleteIsolationLevelKey,
- defVal: IsolationSnapshot,
- want: IsolationSnapshot,
+ name: "unrecognized value returns error",
+ props: iceberg.Properties{WriteDeleteIsolationLevelKey: "repeatable-read"},
+ key: WriteDeleteIsolationLevelKey,
+ defVal: IsolationSerializable,
+ want: IsolationSerializable,
+ wantErr: true,
+ },
+ {
+ name: "empty string returns error",
+ props: iceberg.Properties{WriteDeleteIsolationLevelKey: ""},
+ key: WriteDeleteIsolationLevelKey,
+ defVal: IsolationSnapshot,
+ want: IsolationSnapshot,
+ wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
- got := readIsolationLevel(tt.props, tt.key, tt.defVal)
+ got, err := readIsolationLevel(tt.props, tt.key, tt.defVal)
+ if tt.wantErr {
Review Comment:
this early return skips the `assert.Equal(tt.want, got)` below, so `want` on the two error rows is never actually checked — it's dead. if the impl started returning `IsolationSnapshot` instead of `defVal` on the error path, these tests would still pass.
either drop `want` from the error rows, or assert `got` on the error path too (`assert.Equal(t, tt.want, got)` before the return).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(conflict_validation): reject invalid isolation-level values [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1398:
URL: https://github.com/apache/iceberg-go/pull/1398
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org