FANNG1 commented on code in PR #13929:
URL: https://github.com/apache/gravitino/pull/13929#discussion_r4337295484
##########
gradle/libs.versions.toml:
##########
@@ -32,8 +32,8 @@ lombok = "1.18.20"
slf4j = "2.0.17"
log4j = "2.25.4"
arrow = "18.3.0"
-lance = "6.0.0"
-lance-namespace = "0.7.5"
+lance = "12.0.0"
+lance-namespace = "0.11.1"
Review Comment:
Confirmed and fixed in 332c1da.
You're right about both the mechanism and the blast radius. Three of the
requests Gravitino serves gained a selector — `/describe` (`branch` and `tag`),
`/drop_columns` and `/alter_columns` (`branch`) — and `grep -i branch` over
`lance-rest-server` and `lance-common` main sources returns nothing, so
`{"branch":"dev","columns":["score"]}` passed `validateDropColumnsRequest` and
`GravitinoLanceTableAlterHandler` dropped the column from the table itself.
One correction to the premise, which makes the finding slightly sharper
rather than weaker: the pre-upgrade rejection was not a clean one. The
generated models carry no `@JsonIgnoreProperties(ignoreUnknown = true)` and
`ObjectMapperProvider` leaves Jackson's `FAIL_ON_UNKNOWN_PROPERTIES` on, so the
payload did fail — but it failed during body deserialization, before the
resource method, so it fell through `LanceExceptionMapper`'s `else` branch as a
`500`. The three outcomes are therefore `500` before the upgrade, a silent
mutation of the wrong history as reviewed, and `400` now.
The three handlers reject a non-blank `branch` or `tag` through
`Preconditions.checkArgument`, which `LanceExceptionMapper` maps to
`InvalidInputException` and HTTP `400` — the same shape the file already uses
for `"Only RENAME alteration is supported currently."`. The five operations
that carry `branch` as a query parameter are not served at all, and no
namespace request gained either field.
Regression tests as asked: `testDescribeTableRejectsBranchAndTagSelectors`
and `testAlterRequestsRejectBranchSelector` assert the `400` and the message,
and use `verify(tableOps, never())` so a refused mutation is shown never to
reach the ops layer, which is where a change to the table would have to come
from. The test that previously asserted `200` for a `branch`/`tag` payload
encoded exactly the behaviour you flagged; it is replaced by one that keeps
only the part that still holds (the raw-JSON round trip and the empty response
`context`).
Also checked that nothing in the matrix sends these fields: none of the 263
`org/lance/spark/*` classes in `lance-spark-bundle` 0.8.0 calls `setBranch` or
`setTag`, and the six-bundle matrix and the four-row `lance-ray` matrix are
both green after the change.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceSparkRESTServiceIT.java:
##########
@@ -287,6 +297,58 @@ public void testSelectFromEmptyTableViaSpark() {
Assertions.assertTrue(rows.isEmpty(), "Expected no rows for a newly
created empty table");
}
+ @Test
+ public void testSparkReadsTableCreatedThroughGravitino() {
+ // Creating through Gravitino's own API is the only path that reaches
+ // LanceTableOperations#createDataset, which leaves the Lance file format
version at
+ // lance-core's
+ // default whenever the table has no blob column - 2.2 as of lance-core
12.0.0, where earlier
+ // releases defaulted lower. Spark's CREATE TABLE goes through `declare`
instead, where
+ // Gravitino
+ // writes no dataset at all and the bundle writes it at its own version.
So this is the one
+ // combination that puts a Gravitino-written dataset in front of the
bundle's Lance, which in
+ // this matrix goes back to lance-core 2.0.0.
+ String schemaName = createSchema("spark_gravitino_created");
+ String tableName = newTableName("orders");
+
+ catalog
+ .asTableCatalog()
+ .createTable(
+ NameIdentifier.of(schemaName, tableName),
+ new Column[] {
+ Column.of("id", Types.IntegerType.get()), Column.of("score",
Types.FloatType.get())
+ },
+ "created through the Gravitino API",
+ // Deliberately no LANCE_TABLE_DECLARED: that is the `declare`
path, where Gravitino
+ // writes no dataset and the point of this test is lost.
+ ImmutableMap.of("format", "lance"),
+ Transforms.EMPTY_TRANSFORM,
+ null,
+ null);
+
+ // Reading works for every bundle in the matrix: the file format version
is forward-compatible
+ // on the read path.
+ Assertions.assertTrue(
+ sql("SELECT id, score FROM %s.%s", schemaName, tableName).isEmpty(),
+ "Expected no rows in a freshly created Lance dataset");
Review Comment:
Agreed, and fixed in caa1983.
You're right that the SELECT read a manifest and no data files, so the
cross-version read the PR documents had no coverage. Gravitino's table API
never writes rows, so the test now seeds them itself: `Fragment.write` plus an
`Append` transaction using the test JVM's lance-core — the same 12.0.0 the
server writes with, at its default file format version 2.2 — and then reads
them back through the bundle, whose Lance lives in a child-first class loader.
The unsupported-write check stays separate, as you suggested, and now appends
to a populated dataset.
Worth recording what it measured, because it was not a foregone conclusion:
bundle 0.2.0 (lance-core 2.0.0) reads the seeded 2.2 data files correctly. So
the read path is forward-compatible for data and not only for metadata, which
is what `docs/lakehouse-generic-lance-table.md` had asserted on the strength of
the empty read alone; that claim now points at this test. Only the write breaks
on 2.0.0, so the skip stays where it was, after the seeded read.
Matrix after the change: 15 tests against each of 0.2.0, 0.4.0, 0.5.1,
0.6.1, 0.7.1 and 0.8.0 — 90 tests, 0 failures, the one deliberate skip on 0.2.0.
--
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]
