Copilot commented on code in PR #11060:
URL: https://github.com/apache/gravitino/pull/11060#discussion_r3238608731
##########
clients/client-python/build.gradle.kts:
##########
@@ -239,6 +239,52 @@ tasks {
finalizedBy(unitCoverageReport)
}
+ // Run tests/integration/test_lance_ray.py against multiple lance-ray
+ // versions. Each version is exercised inside its own venv under
+ // build/lance-ray-matrix/.venv-<version>/ (cached across runs).
+ // Override the matrix with `-PlanceRayVersions=0.4.2,0.4.1,0.4.0`.
+ register("lanceRayMatrixTest") {
+ group = "verification"
+ description =
+ "Run tests/integration/test_lance_ray.py against multiple lance-ray " +
+ "versions. Override with -PlanceRayVersions=<csv> (default: " +
+ "tracks docs/lance-rest-integration.md Compatibility Matrix)."
+
+ val versions = project.findProperty("lanceRayVersions") as? String
+ val keepGoing = project.hasProperty("lanceRayKeepGoing")
+ val script = projectDir.resolve("scripts/run_lance_ray_matrix.py")
+ val gravitinoHome = file("${project.rootDir}/distribution/package")
+
+ doFirst {
+ gravitinoServer("start")
+ }
+ doLast {
+ try {
+ val args = mutableListOf(
+ "python3",
+ script.absolutePath,
+ "--gravitino-home",
+ gravitinoHome.absolutePath,
+ )
Review Comment:
The lanceRayMatrixTest task hard-codes "python3" when launching
scripts/run_lance_ray_matrix.py. This bypasses the miniforge
pythonPlugin/VenvTask toolchain used elsewhere in this module and can fail in
environments where python3 is not on PATH (or where the build expects the
provisioned interpreter). Consider invoking the script via the plugin-managed
interpreter / a VenvTask, or at least make the executable configurable (similar
to pythonPlugin.pythonVersion).
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/JsonNullableMapperProvider.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.lance.service.rest;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import javax.ws.rs.ext.ContextResolver;
+import javax.ws.rs.ext.Provider;
+import org.openapitools.jackson.nullable.JsonNullableModule;
+
+/**
+ * JAX-RS {@link ContextResolver} that provides an {@link ObjectMapper} with
the {@link
+ * JsonNullableModule} registered.
+ *
+ * <p>lance-namespace 0.7.5 models use {@code JsonNullable<T>} for optional
fields, which requires
+ * this module for correct Jackson serialization/deserialization.
+ */
+@Provider
+public class JsonNullableMapperProvider implements
ContextResolver<ObjectMapper> {
+
+ private static final ObjectMapper MAPPER =
+ new ObjectMapper().registerModule(new JsonNullableModule());
+
+ @Override
+ public ObjectMapper getContext(Class<?> type) {
+ return MAPPER;
Review Comment:
JsonNullableMapperProvider returns a brand-new ObjectMapper with only
JsonNullableModule registered. This drops the shared Gravitino Jackson
configuration (e.g., JavaTimeModule/Jdk8Module and enum/lowercase settings in
server-common ObjectMapperProvider), which can change JSON
serialization/deserialization behavior across endpoints. Consider basing this
mapper on org.apache.gravitino.server.web.ObjectMapperProvider.objectMapper()
(e.g., copy it and then register JsonNullableModule) so lance-rest keeps
consistent JSON settings while adding JsonNullable support.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -164,7 +163,7 @@ public Response createEmptyTable(
// conflict.
props.putAll(headerProps);
- CreateEmptyTableResponse response =
+ DeclareTableResponse response =
lanceNamespace.asTableOps().createEmptyTable(tableId, delimiter,
tableLocation, props);
return Response.ok(response).build();
Review Comment:
The Javadoc for the createEmptyTable endpoint references "lance-namespace
... 0.0.20 to 0.31", but this module is now upgraded to lance-namespace 0.7.5.
Please update the comment to reflect the current supported versions / rationale
for keeping this legacy endpoint (or remove the version-specific range) to
avoid misleading future maintainers.
##########
clients/client-python/tests/integration/test_lance_ray.py:
##########
@@ -0,0 +1,316 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+import os
+import shutil
+import tempfile
+import time
+import unittest
+from random import randint
+from typing import Optional
+
+import requests
+
+from gravitino import (
+ Catalog,
+ GravitinoAdminClient,
+ GravitinoClient,
+)
+from tests.integration.integration_test_env import IntegrationTestEnv
+
+logger = logging.getLogger(__name__)
+
+LANCE_REST_PORT = 9101
+LANCE_REST_BASE_URL = f"http://localhost:{LANCE_REST_PORT}/lance";
+
+# The Lance REST server runs as an auxiliary service inside the main
+# Gravitino process (gravitino.auxService.names = ...,lance-rest), so its
+# bind metalake is configured in the *main* gravitino.conf rather than the
+# standalone lance-rest conf file.
+MAIN_CONF_FILE = "conf/gravitino.conf"
+LANCE_REST_METALAKE_KEY = "gravitino.lance-rest.gravitino-metalake"
+
+
+def _missing_lance_ray_deps() -> Optional[str]:
+ missing = []
+ for mod in ("ray", "lance_ray", "lance_namespace"):
+ try:
+ __import__(mod)
+ except ImportError:
+ missing.append(mod)
+ return ", ".join(missing) if missing else None
+
+
[email protected](
+ _missing_lance_ray_deps() is not None,
+ f"lance-ray test deps not installed: {_missing_lance_ray_deps()}. "
+ "Install with: pip install ray lance-ray lance-namespace",
+)
+class TestLanceRayIntegration(IntegrationTestEnv):
+ """End-to-end test for the lance-ray Python client against a
Gravitino-backed
+ Lance REST namespace. Mirrors the ``ray.data`` -> ``write_lance`` ->
+ ``read_lance`` flow from the upstream lance-ray docs.
+ """
+
+ # Metalake name is fixed (not randomized) so back-to-back runs in the
+ # same Gravitino process can detect that the lance-rest aux service is
+ # already bound and skip the costly server restart. The per-test table
+ # name still gets a random suffix to keep individual test methods
+ # isolated.
+ METALAKE_NAME: str = "lance_ray_test_metalake"
+ CATALOG_NAME: str = "lance_catalog"
+ SCHEMA_NAME: str = "schema"
+ TABLE_NAME: str = "lance_ray_tbl_" + str(randint(1, 100000))
+
+ gravitino_admin_client: Optional[GravitinoAdminClient] = None
+ gravitino_client: Optional[GravitinoClient] = None
+ temp_dir: Optional[str] = None
+ main_conf_path: Optional[str] = None
+
+ @classmethod
+ def setUpClass(cls):
+ super().setUpClass()
+
+ gravitino_home = os.environ.get("GRAVITINO_HOME")
+ if not gravitino_home:
+ raise RuntimeError(
+ "GRAVITINO_HOME must be set to the distribution package
directory"
+ )
+ cls.main_conf_path = os.path.join(gravitino_home, MAIN_CONF_FILE)
+
+ # Bind the lance-rest aux service to our test metalake. If the same
+ # binding is already present (e.g. an earlier run in the same Gradle
+ # session left it there), skip the conf write and the restart. This
+ # avoids restarting Gravitino in the middle of the IT suite when the
+ # test class is replayed, which would briefly disrupt other ITs.
+ if not cls._lance_metalake_already_bound():
+ cls._append_conf(
+ {LANCE_REST_METALAKE_KEY: cls.METALAKE_NAME},
cls.main_conf_path
+ )
+ cls.restart_server()
+ if not cls._wait_for_lance_rest_ready():
+ raise RuntimeError(
+ "Lance REST aux service did not become ready in time at "
+ + LANCE_REST_BASE_URL
+ )
+
+ # Probe whether the server-side `lance-namespace-core` is new enough
+ # to deserialize requests from the installed PyPI `lance-namespace`.
+ # We skip cleanly (rather than fail) when the server is older — this
+ # happens on branches that haven't merged the `lance-namespace-core`
+ # upgrade yet. The probe runs *before* metalake/catalog/schema setup
+ # so a skipped run leaves no fixtures behind.
+ skip_reason = cls._check_lance_namespace_compat()
+ if skip_reason is not None:
+ raise unittest.SkipTest(skip_reason)
+
+ cls.gravitino_admin_client =
GravitinoAdminClient("http://localhost:8090";)
+ # Idempotent: tolerate a metalake left over from a prior failed run.
+ try:
+ cls.gravitino_admin_client.create_metalake(
+ cls.METALAKE_NAME,
+ comment="lance-ray IT metalake",
+ properties={},
+ )
+ except Exception as e: # pylint: disable=broad-exception-caught
+ if "already exists" not in str(e).lower():
+ raise
+ logger.info("Metalake %s already exists, reusing",
cls.METALAKE_NAME)
+ cls.gravitino_client = GravitinoClient(
+ uri="http://localhost:8090";, metalake_name=cls.METALAKE_NAME
+ )
+ cls.temp_dir = tempfile.mkdtemp(prefix="lance_ray_it_")
+ # Idempotent catalog + schema creation too.
+ try:
+ cls.gravitino_client.create_catalog(
+ name=cls.CATALOG_NAME,
+ catalog_type=Catalog.Type.RELATIONAL,
+ provider="lakehouse-generic",
+ comment="lance-ray IT catalog",
+ properties={"location": cls.temp_dir},
+ )
+ except Exception as e: # pylint: disable=broad-exception-caught
+ if "already exists" not in str(e).lower():
+ raise
+ catalog = cls.gravitino_client.load_catalog(cls.CATALOG_NAME)
+ try:
+ catalog.as_schemas().create_schema(
+ schema_name=cls.SCHEMA_NAME,
+ comment="lance-ray IT schema",
+ properties={},
+ )
+ except Exception as e: # pylint: disable=broad-exception-caught
+ if "already exists" not in str(e).lower():
+ raise
+
+ @classmethod
+ def tearDownClass(cls):
+ failures = []
+
+ try:
+ if cls.gravitino_client is not None:
+ cls.gravitino_client.drop_catalog(name=cls.CATALOG_NAME,
force=True)
+ except Exception as e: # pylint: disable=broad-exception-caught
+ failures.append(("drop catalog", e))
+
+ try:
+ if cls.gravitino_admin_client is not None:
+ cls.gravitino_admin_client.drop_metalake(
+ name=cls.METALAKE_NAME, force=True
+ )
+ except Exception as e: # pylint: disable=broad-exception-caught
+ failures.append(("drop metalake", e))
+
+ # Intentionally do NOT reset the lance-rest metalake binding in
+ # gravitino.conf. Removing it would force the next setUpClass to
+ # restart the server, which is disruptive when this IT is replayed
+ # back-to-back or alongside other ITs in the same Gradle invocation.
+ # The conf line is regenerated by `compileDistribution`, so it does
+ # not survive a fresh distribution build.
+
+ try:
+ if cls.temp_dir and os.path.exists(cls.temp_dir):
+ shutil.rmtree(cls.temp_dir, ignore_errors=True)
+ except Exception as e: # pylint: disable=broad-exception-caught
+ failures.append(("remove temp dir", e))
+
+ for step, err in failures:
+ logger.warning("Cleanup step %s failed: %s", step, err)
+
+ super().tearDownClass()
+
+ @classmethod
+ def _check_lance_namespace_compat(cls) -> Optional[str]:
+ """Detect server/client schema drift on lance-namespace.
+
+ The lance-namespace REST model evolves: newer client versions add
+ request fields (e.g. ``check_declared``) that older
+ ``lance-namespace-core`` builds on the server side reject with
+ Jackson's "Unrecognized field ... not marked as ignorable". This
+ helper sends a harmless ``describe_table`` for a bogus table id so
+ we can observe the schema-validation error without doing any real
+ work. Returns a skip reason on incompatibility, or ``None`` if the
+ server understands the request shape.
+ """
+ try:
+ # pylint: disable=import-outside-toplevel
+ import lance_namespace
+ from lance_namespace import DescribeTableRequest
+
+ # pylint: enable=import-outside-toplevel
+ except ImportError:
+ # `@unittest.skipIf` on the class already handles this case; if
+ # we reach here something odd is going on but it's not our job
+ # to recover from it.
+ return None
+
+ try:
+ ns = lance_namespace.connect("rest", {"uri": LANCE_REST_BASE_URL})
+ except Exception as e: # pylint: disable=broad-exception-caught
+ return f"unable to connect to lance-rest aux service: {e}"
+
+ probe = DescribeTableRequest(id=["__probe__", "__probe__",
"__probe__"])
+ try:
+ ns.describe_table(probe)
+ # Unlikely but not impossible: the probe table actually exists
+ # in a leftover metalake. That's still a compatible server.
+ return None
+ except Exception as e: # pylint: disable=broad-exception-caught
+ msg = str(e)
+ if "Unrecognized field" in msg or "not marked as ignorable" in msg:
+ short = msg.splitlines()[0][:200]
+ return (
+ "lance-rest server's lance-namespace-core is older than "
+ "the client's lance-namespace (request schema mismatch). "
+ f"Server reported: {short}. To run this test, upgrade "
+ "the server (e.g. via PR #11060 -> lance-namespace 0.7.5) "
+ "or roll the client back to a matching version."
Review Comment:
The skip reason hard-codes a specific PR reference ("PR #11060") and a
specific server upgrade path. This will become stale quickly and is confusing
when the test is run from other branches or after the upgrade has landed.
Consider rephrasing the message to describe the required server-side
lance-namespace-core version (e.g., 0.7.5+) without referencing a particular PR
number.
##########
clients/client-python/tests/integration/test_lance_ray.py:
##########
@@ -0,0 +1,316 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+import os
+import shutil
+import tempfile
+import time
+import unittest
+from random import randint
+from typing import Optional
+
+import requests
+
+from gravitino import (
+ Catalog,
+ GravitinoAdminClient,
+ GravitinoClient,
+)
+from tests.integration.integration_test_env import IntegrationTestEnv
+
+logger = logging.getLogger(__name__)
+
+LANCE_REST_PORT = 9101
+LANCE_REST_BASE_URL = f"http://localhost:{LANCE_REST_PORT}/lance";
+
+# The Lance REST server runs as an auxiliary service inside the main
+# Gravitino process (gravitino.auxService.names = ...,lance-rest), so its
+# bind metalake is configured in the *main* gravitino.conf rather than the
+# standalone lance-rest conf file.
+MAIN_CONF_FILE = "conf/gravitino.conf"
+LANCE_REST_METALAKE_KEY = "gravitino.lance-rest.gravitino-metalake"
+
+
+def _missing_lance_ray_deps() -> Optional[str]:
+ missing = []
+ for mod in ("ray", "lance_ray", "lance_namespace"):
+ try:
+ __import__(mod)
+ except ImportError:
+ missing.append(mod)
+ return ", ".join(missing) if missing else None
+
+
[email protected](
+ _missing_lance_ray_deps() is not None,
+ f"lance-ray test deps not installed: {_missing_lance_ray_deps()}. "
+ "Install with: pip install ray lance-ray lance-namespace",
+)
+class TestLanceRayIntegration(IntegrationTestEnv):
+ """End-to-end test for the lance-ray Python client against a
Gravitino-backed
+ Lance REST namespace. Mirrors the ``ray.data`` -> ``write_lance`` ->
+ ``read_lance`` flow from the upstream lance-ray docs.
+ """
+
+ # Metalake name is fixed (not randomized) so back-to-back runs in the
+ # same Gravitino process can detect that the lance-rest aux service is
+ # already bound and skip the costly server restart. The per-test table
+ # name still gets a random suffix to keep individual test methods
+ # isolated.
+ METALAKE_NAME: str = "lance_ray_test_metalake"
+ CATALOG_NAME: str = "lance_catalog"
+ SCHEMA_NAME: str = "schema"
+ TABLE_NAME: str = "lance_ray_tbl_" + str(randint(1, 100000))
+
+ gravitino_admin_client: Optional[GravitinoAdminClient] = None
+ gravitino_client: Optional[GravitinoClient] = None
+ temp_dir: Optional[str] = None
+ main_conf_path: Optional[str] = None
+
+ @classmethod
+ def setUpClass(cls):
+ super().setUpClass()
+
+ gravitino_home = os.environ.get("GRAVITINO_HOME")
+ if not gravitino_home:
+ raise RuntimeError(
+ "GRAVITINO_HOME must be set to the distribution package
directory"
+ )
+ cls.main_conf_path = os.path.join(gravitino_home, MAIN_CONF_FILE)
+
+ # Bind the lance-rest aux service to our test metalake. If the same
+ # binding is already present (e.g. an earlier run in the same Gradle
+ # session left it there), skip the conf write and the restart. This
+ # avoids restarting Gravitino in the middle of the IT suite when the
+ # test class is replayed, which would briefly disrupt other ITs.
+ if not cls._lance_metalake_already_bound():
+ cls._append_conf(
+ {LANCE_REST_METALAKE_KEY: cls.METALAKE_NAME},
cls.main_conf_path
+ )
+ cls.restart_server()
+ if not cls._wait_for_lance_rest_ready():
+ raise RuntimeError(
+ "Lance REST aux service did not become ready in time at "
+ + LANCE_REST_BASE_URL
+ )
+
+ # Probe whether the server-side `lance-namespace-core` is new enough
+ # to deserialize requests from the installed PyPI `lance-namespace`.
+ # We skip cleanly (rather than fail) when the server is older — this
+ # happens on branches that haven't merged the `lance-namespace-core`
+ # upgrade yet. The probe runs *before* metalake/catalog/schema setup
+ # so a skipped run leaves no fixtures behind.
+ skip_reason = cls._check_lance_namespace_compat()
+ if skip_reason is not None:
+ raise unittest.SkipTest(skip_reason)
+
+ cls.gravitino_admin_client =
GravitinoAdminClient("http://localhost:8090";)
+ # Idempotent: tolerate a metalake left over from a prior failed run.
+ try:
+ cls.gravitino_admin_client.create_metalake(
+ cls.METALAKE_NAME,
+ comment="lance-ray IT metalake",
+ properties={},
+ )
+ except Exception as e: # pylint: disable=broad-exception-caught
+ if "already exists" not in str(e).lower():
+ raise
+ logger.info("Metalake %s already exists, reusing",
cls.METALAKE_NAME)
+ cls.gravitino_client = GravitinoClient(
+ uri="http://localhost:8090";, metalake_name=cls.METALAKE_NAME
+ )
+ cls.temp_dir = tempfile.mkdtemp(prefix="lance_ray_it_")
+ # Idempotent catalog + schema creation too.
+ try:
+ cls.gravitino_client.create_catalog(
+ name=cls.CATALOG_NAME,
+ catalog_type=Catalog.Type.RELATIONAL,
+ provider="lakehouse-generic",
+ comment="lance-ray IT catalog",
+ properties={"location": cls.temp_dir},
+ )
+ except Exception as e: # pylint: disable=broad-exception-caught
+ if "already exists" not in str(e).lower():
+ raise
+ catalog = cls.gravitino_client.load_catalog(cls.CATALOG_NAME)
+ try:
+ catalog.as_schemas().create_schema(
+ schema_name=cls.SCHEMA_NAME,
+ comment="lance-ray IT schema",
+ properties={},
+ )
+ except Exception as e: # pylint: disable=broad-exception-caught
+ if "already exists" not in str(e).lower():
+ raise
+
+ @classmethod
+ def tearDownClass(cls):
+ failures = []
+
+ try:
+ if cls.gravitino_client is not None:
+ cls.gravitino_client.drop_catalog(name=cls.CATALOG_NAME,
force=True)
+ except Exception as e: # pylint: disable=broad-exception-caught
+ failures.append(("drop catalog", e))
+
+ try:
+ if cls.gravitino_admin_client is not None:
+ cls.gravitino_admin_client.drop_metalake(
+ name=cls.METALAKE_NAME, force=True
+ )
+ except Exception as e: # pylint: disable=broad-exception-caught
+ failures.append(("drop metalake", e))
+
+ # Intentionally do NOT reset the lance-rest metalake binding in
+ # gravitino.conf. Removing it would force the next setUpClass to
+ # restart the server, which is disruptive when this IT is replayed
+ # back-to-back or alongside other ITs in the same Gradle invocation.
+ # The conf line is regenerated by `compileDistribution`, so it does
+ # not survive a fresh distribution build.
Review Comment:
tearDownClass intentionally does not reset the gravitino.conf change
(lance-rest metalake binding). Other Python integration tests in this repo
typically call IntegrationTestEnv._reset_conf in tearDownClass to avoid leaking
config changes across the IT suite. Leaving the binding behind can make later
tests depend on this class’s execution order and may require manual cleanup
when running subsets locally. Consider restoring the previous config (only if
this test appended it) and restarting the server, or gating the no-reset
behavior behind an explicit opt-in flag used only for the matrix runner.
--
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]
