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: [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]
