|
| 1 | +--- |
| 2 | +name: proto-compat-audit |
| 3 | +description: Audit Java proto definitions and encoding metadata field tags against the Rust spiraldb/vortex reference implementation. Run whenever upstream vortex evolves. No formal spec exists — Rust source is ground truth. |
| 4 | +--- |
| 5 | + |
| 6 | +## Overview |
| 7 | + |
| 8 | +This skill audits three things: |
| 9 | + |
| 10 | +1. `dtype.proto` + `scalar.proto` field-by-field against upstream `spiraldb/vortex` |
| 11 | +2. Every encoding metadata message in `encodings.proto` against the Rust `from_proto`/`to_proto` impls |
| 12 | +3. Integration test assertions — must check actual decoded values, not just row counts |
| 13 | + |
| 14 | +Proto3 silently returns defaults (0 / "" / false) on tag mismatch. Wrong tag = wrong decode = silent corruption. |
| 15 | +Rust reference is at `https://github.com/spiraldb/vortex`. Fetch via `gh api repos/spiraldb/vortex/contents/<path>`. |
| 16 | + |
| 17 | +--- |
| 18 | + |
| 19 | +## Step 1 — Sync dtype.proto and scalar.proto |
| 20 | + |
| 21 | +Fetch upstream versions: |
| 22 | + |
| 23 | +``` |
| 24 | +gh api repos/spiraldb/vortex/contents/vortex-proto/proto/dtype.proto --jq '.content' | base64 -d |
| 25 | +gh api repos/spiraldb/vortex/contents/vortex-proto/proto/scalar.proto --jq '.content' | base64 -d |
| 26 | +``` |
| 27 | + |
| 28 | +Diff against local `core/src/main/proto/dtype.proto` and `core/src/main/proto/scalar.proto`. |
| 29 | + |
| 30 | +Check for: |
| 31 | +- New fields added upstream (new field numbers we don't handle) |
| 32 | +- Removed or renumbered fields (tag mismatch → silent zero) |
| 33 | +- New enum values in `PType` or `DType.oneof` |
| 34 | +- Changes to `ScalarValue.oneof` |
| 35 | + |
| 36 | +If diverged: update local `.proto`, regenerate (`./mvnw generate-sources -pl core -P regenerate-sources`), update any Java code that reads the affected fields, add a test. |
| 37 | + |
| 38 | +--- |
| 39 | + |
| 40 | +## Step 2 — Audit encoding metadata field tags |
| 41 | + |
| 42 | +`encodings.proto` has no upstream `.proto` counterpart. Field numbers were assigned manually. |
| 43 | +Rust source of truth: `vortex-encodings/<encoding>/src/` — look for `MetadataAny`, `from_proto`, `to_proto`, or `Metadata` structs with protobuf derives. |
| 44 | + |
| 45 | +Upstream proto path for reference (may not exist for all): `gh api repos/spiraldb/vortex/contents/vortex-encodings/<name>/src/`. |
| 46 | + |
| 47 | +For each message below, verify field tag numbers match what Rust serializes. Silent corruption risk ranked high→low: |
| 48 | + |
| 49 | +| Message | Key fields | Corruption if wrong | |
| 50 | +|---|---|---| |
| 51 | +| `RLEMetadata` | `values_len`(1), `indices_len`(2), `indices_ptype`(3) | empty/wrong-typed decode | |
| 52 | +| `RunEndMetadata` | `ends_ptype`(1), `num_runs`(2), `offset`(3) | no data | |
| 53 | +| `ALPMetadata` | `exp_e`(1), `exp_f`(2) | all zeroes | |
| 54 | +| `BitPackedMetadata` | `bit_width`(1), `offset`(2) | garbage unpack | |
| 55 | +| `DictMetadata` | `values_len`(1), `codes_ptype`(2) | empty dict | |
| 56 | +| `ALPRDMetadata` | `right_bit_width`(1), `dict_len`(2), `dict`(3), `left_parts_ptype`(4) | wrong reconstruction | |
| 57 | +| `PcoMetadata` | `header`(1), `chunks`(2) | full decode failure | |
| 58 | +| `SparseMetadata` | `patches`(1) | missing patches | |
| 59 | +| `DateTimePartsMetadata` | `days_ptype`(1), `seconds_ptype`(2), `subseconds_ptype`(3) | wrong datetime | |
| 60 | +| `DeltaMetadata` | `deltas_len`(1), `offset`(2) | wrong deltas | |
| 61 | +| `ZstdMetadata` | `dictionary_size`(1), `frames`(2) | wrong sizes | |
| 62 | +| `DictMetadata` | `is_nullable_codes`(3), `all_values_referenced`(4) | wrong nullability | |
| 63 | +| `VarBinMetadata` | `offsets_ptype`(1) | wrong offsets | |
| 64 | +| `FSSTMetadata` | `uncompressed_lengths_ptype`(1), `codes_offsets_ptype`(2) | wrong string decode | |
| 65 | +| `SequenceMetadata` | `base`(1), `multiplier`(2) | wrong sequence | |
| 66 | + |
| 67 | +For any mismatch: fix field number in `encodings.proto`, regenerate, update decoder, add regression test with a pinned Rust-produced fixture that exercises the field. |
| 68 | + |
| 69 | +--- |
| 70 | + |
| 71 | +## Step 3 — Strengthen integration test assertions |
| 72 | + |
| 73 | +File: `integration/src/test/java/.../RustWritesJavaReadsIntegrationTest.java` |
| 74 | + |
| 75 | +### Tests that only check rowCount — upgrade to value assertions: |
| 76 | + |
| 77 | +**`jniWriter_nullableColumn_decodesWithoutError`** — currently checks rowCount + dtype only. |
| 78 | +Add: verify non-null positions have correct values (`i % 5 != 0` → value == i), verify null positions return appropriate null indicator. |
| 79 | + |
| 80 | +**`jniWriter_javaReader_fewUniqueF64Values`** — sum check is weak (values that sum correctly can still be wrong). |
| 81 | +Add: assert first N decoded values match exactly `[1.1, 2.2, 3.3, 1.1, 2.2, ...]`. |
| 82 | + |
| 83 | +### S3 `javaDecodeMatchesJni` tests — add pinned golden values: |
| 84 | + |
| 85 | +Tests at `s3_pcoVortex_javaDecodeMatchesJni`, `s3_tpchLineitem_javaDecodeMatchesJni`, `s3_tpchOrders_javaDecodeMatchesJni`, `s3_clickbenchHits5k_javaDecodeMatchesJni` compare Java vs JNI — both could be wrong together. |
| 86 | + |
| 87 | +Add at least one **pinned golden value per test**: a specific row index and its expected value, derived independently (e.g., from the Rust CLI `vortex inspect` or by reading the file with the Rust binary separately). This anchors the test to ground truth, not just Java-JNI consistency. |
| 88 | + |
| 89 | +--- |
| 90 | + |
| 91 | +## Step 4 — Unit-level metadata round-trip tests |
| 92 | + |
| 93 | +For each encoding that uses protobuf metadata, add or extend the unit test class with a `Metadata` nested class: |
| 94 | + |
| 95 | +```java |
| 96 | +@Nested |
| 97 | +class Metadata { |
| 98 | + @Test |
| 99 | + void roundTrips() { |
| 100 | + // Given — encode known data |
| 101 | + EncodeResult result = new FooEncoding().encode(dtype, data); |
| 102 | + // When — parse metadata bytes |
| 103 | + FooMetadataProto meta = FooMetadataProto.parseFrom(result.node().metadata()); |
| 104 | + // Then — assert every non-default field |
| 105 | + assertThat(meta.getBitWidth()).isEqualTo(expectedBitWidth); |
| 106 | + assertThat(meta.getOffset()).isEqualTo(expectedOffset); |
| 107 | + } |
| 108 | +} |
| 109 | +``` |
| 110 | + |
| 111 | +Encodings requiring this test (add if missing): `AlpEncoding`, `AlpRdEncoding`, `BitpackedEncoding`, `DateTimePartsEncoding`, `DeltaEncoding`, `DictEncoding`, `FsstEncoding`, `PcoEncoding`, `RleEncoding`, `RunEndEncoding`, `SparseEncoding`, `VarBinEncoding`, `ZstdEncoding`. |
| 112 | + |
| 113 | +--- |
| 114 | + |
| 115 | +## Completion criteria |
| 116 | + |
| 117 | +- [ ] `dtype.proto` and `scalar.proto` match upstream (or divergences are documented with a reason) |
| 118 | +- [ ] Every encoding metadata message field tag verified against Rust source |
| 119 | +- [ ] No integration test asserts only `rowCount` — all assert actual values |
| 120 | +- [ ] Every metadata-using encoding has a unit-level metadata round-trip test |
| 121 | +- [ ] `./mvnw verify` passes |
0 commit comments