Conversation
17b5c27 to
fbccef8
Compare
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Two changes to the conversion and one to the test. The fix for #1175 itself is right, and it matches the ordinal the outbound direction already emits, so this makes the two directions agree rather than picking a reading; the adjacent WHERE-clause gap is covered by your #1259.
One point sits outside the diff, so no suggestion is attached: core/src/main/java/io/substrait/relation/AbstractUpdate.java:53. getColumnTarget()'s Javadoc says only "the index of the target column to update", while NamedStruct.names() advertises the flattened depth-first list — the list this bug came from reaching for. The spec says only "index of the column to apply the transformation to" and never says which of the two it indexes, so it is worth stating on the accessor that it is a top-level ordinal into getTableSchema().struct(), and that this is the library's reading rather than something the spec settles.
For a table src(s ROW(x INTEGER), x INTEGER, n INTEGER), UPDATE src SET n = 99 converts back to UPDATE src SET x = 99. The transform's top-level column ordinal is incorrectly used to index the flattened depth-first name list [S, X, X, N]. Reconstruct the declared row type before resolving target names so nested fields do not shift top-level update columns. This also preserves multiple-assignment order when nested and top-level names collide. Partially addresses substrait-io#1175; struct-literal assignment field names are separate from target-column resolution.
fbccef8 to
0b419ef
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesUPDATE target conversion
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The UPDATE conversion maps valid targets to top-level fields and rejects invalid ordinals. No actionable merge-blocking risk remains in the reviewed change. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change corrects which column a nested-schema UPDATE targets, and no new security bypass was identified. Its effect on actual writes still depends on authorization and execution controls outside the reviewed conversion path. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
One small follow-up on the bounds check.
| assertEquals(List.of("N"), converted.getUpdateColumnList()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Pin the guard with a test — nothing currently fails if it is removed, so a later refactor can drop it silently. I checked: deleting the bounds check leaves the rest of this file green.
| assertEquals(List.of("N"), converted.getUpdateColumnList()); | |
| } | |
| } | |
| assertEquals(List.of("N"), converted.getUpdateColumnList()); | |
| } | |
| @Test | |
| void rejectsOutOfRangeColumnTarget() throws Exception { | |
| Prepare.CatalogReader catalog = | |
| SubstraitCreateStatementParser.processCreateStatementsToCatalog( | |
| "CREATE TABLE src (u INTEGER, n INTEGER)"); | |
| Plan plan = new SqlToSubstrait().convert("UPDATE src SET n = 11", catalog); | |
| NamedUpdate update = assertInstanceOf(NamedUpdate.class, plan.getRoots().get(0).getInput()); | |
| for (int outOfRange : new int[] {-1, 2}) { | |
| AbstractUpdate.TransformExpression transform = | |
| AbstractUpdate.TransformExpression.builder() | |
| .from(update.getTransformations().get(0)) | |
| .columnTarget(outOfRange) | |
| .build(); | |
| NamedUpdate foreignUpdate = | |
| ImmutableNamedUpdate.copyOf(update).withTransformations(List.of(transform)); | |
| IllegalArgumentException e = | |
| org.junit.jupiter.api.Assertions.assertThrows( | |
| IllegalArgumentException.class, | |
| () -> | |
| new SubstraitToCalcite(ConverterProvider.DEFAULT, catalog) | |
| .convert(foreignUpdate)); | |
| assertEquals( | |
| "Update column target " | |
| + outOfRange | |
| + " is outside the table schema's 2 top-level columns", | |
| e.getMessage()); | |
| } | |
| } | |
| } |
assertThrows is spelled out in full here only so the block applies on its own; a static import alongside the other two reads better.
2 participants
For a table src(s ROW(x INTEGER), x INTEGER, n INTEGER), UPDATE src SET n = 99 converts back to UPDATE src SET x = 99. The transform's top-level column ordinal is incorrectly used to index the flattened depth-first name list [S, X, X, N].
Resolve target names from top-level fields in the declared schema without converting untouched types. This preserves multiple-assignment order when nested and top-level names collide. Document the library convention that target indexes address top-level struct fields, rather than the flattened name list.
Partially addresses #1175; struct-literal assignment field names are separate from target-column resolution.
Summary by CodeRabbit