Copilot commented on code in PR #13929:
URL: https://github.com/apache/gravitino/pull/13929#discussion_r4332280496
##########
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:
The upgraded models accept `branch` on mutation requests, but the handlers
ignore it. For example, `/drop_columns` with
`{"branch":"dev","columns":["score"]}` passes `validateDropColumnsRequest`, and
`GravitinoLanceTableAlterHandler.DropColumns` turns it into a column deletion
on the ordinary table. A request targeting `dev` can therefore change main
instead; previously the unknown field caused rejection. Reject unsupported
branch selectors before dispatch, and unsupported `tag` selectors on describe,
rather than silently ignoring them. Add regression tests confirming rejected
mutations leave main unchanged.
##########
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:
This SELECT reads an empty dataset, so it does not test decoding data files
written by lance-core 12.0.0. Bundle 0.2.0 skips before INSERT, and the other
bundles subsequently read files they wrote themselves. The newly documented
cross-version read compatibility therefore remains untested. Seed a populated
dataset with the server's 12.0.0 writer and assert its values through every
bundle before the write-only skip, keeping the unsupported-write check separate.
--
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]
