yuqi1129 commented on code in PR #11060: URL: https://github.com/apache/gravitino/pull/11060#discussion_r3272232565
########## clients/client-python/tests/integration/test_lance_ray.py: ########## @@ -0,0 +1,335 @@ +# 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 + + +# Compute once at module import time so the @skipIf condition and message +# don't trigger two rounds of import attempts. +_MISSING_LANCE_RAY_DEPS = _missing_lance_ray_deps() + + [email protected]( + _MISSING_LANCE_RAY_DEPS is not None, + f"lance-ray test deps not installed: {_MISSING_LANCE_RAY_DEPS}. " + "Install with: pip install -e .[lance] (or pip install ray lance-ray " + "lance-namespace). Requires the Gravitino server to expose a lance-rest " + "auxiliary service backed by lance-namespace-core >= 0.7.5.", +) +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 because the lance-rest aux service binds to a + # single metalake from gravitino.conf. 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 + appended_lance_rest_conf: bool = False + + @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" + ) Review Comment: Updated in 4b4442d48: `setUpClass` now uses the existing `_get_gravitino_home()` behavior instead of raising a RuntimeError directly. ########## lance/lance-common/src/main/java/org/apache/gravitino/lance/common/utils/LancePropertiesUtils.java: ########## @@ -68,4 +68,14 @@ public static Map<String, String> resolveLanceStorageOptions( effectiveStorageOptions.putAll(getLanceStorageOptions(tableProperties)); return effectiveStorageOptions; } + + public static Map<String, String> toTableProperties(Map<String, String> storageOptions) { + if (storageOptions == null) { + return Map.of(); + } + + return storageOptions.entrySet().stream() + .collect( + Collectors.toMap(e -> LANCE_STORAGE_OPTIONS_PREFIX + e.getKey(), Map.Entry::getValue)); + } Review Comment: Updated in 4b4442d48: added Javadoc for the public helper. ########## lance/lance-common/src/main/java/org/apache/gravitino/lance/common/utils/LancePropertiesUtils.java: ########## @@ -68,4 +68,14 @@ public static Map<String, String> resolveLanceStorageOptions( effectiveStorageOptions.putAll(getLanceStorageOptions(tableProperties)); return effectiveStorageOptions; } + + public static Map<String, String> toTableProperties(Map<String, String> storageOptions) { + if (storageOptions == null) { + return Map.of(); + } + + return storageOptions.entrySet().stream() + .collect( + Collectors.toMap(e -> LANCE_STORAGE_OPTIONS_PREFIX + e.getKey(), Map.Entry::getValue)); + } Review Comment: Updated in 4b4442d48: simplified the null handling and added Javadoc for `toTableProperties`. ########## core/src/main/java/org/apache/gravitino/stats/storage/LancePartitionStatisticStorage.java: ########## @@ -203,11 +204,12 @@ public LancePartitionStatisticStorage(Map<String, String> properties) { Caffeine.newBuilder() .maximumSize(datasetCacheSize) .scheduler(Scheduler.forScheduledExecutorService(this.scheduler)) - .evictionListener( - (RemovalListener<Long, Dataset>) + .executor(Runnable::run) + .removalListener( + (RemovalListener<Long, DatasetHolder>) (key, value, cause) -> { if (value != null) { - value.close(); + closeDatasetHolder(value); } }) Review Comment: Updated in 4b4442d48: removed the synchronous Caffeine executor, made holder close idempotent, and explicitly closes cached holders during storage shutdown before closing the allocator. ########## core/build.gradle.kts: ########## @@ -54,6 +54,13 @@ dependencies { exclude(group = "com.fasterxml.jackson.jaxrs", module = "jackson-jaxrs-json-provider") // using gravitino's version exclude(group = "org.apache.httpcomponents.client5", module = "*") // provided by gravitino exclude(group = "org.lance", module = "lance-namespace-core") // This is unnecessary in the core module + // Same rationale as lance-namespace-core: lance-core 4.0.1 declares + // lance-namespace-apache-client as a transitive, but core never calls into it. + // Leaving it on the main classpath shadows the lance-rest aux service's own + // lance-namespace-apache-client (loaded via lance-rest-server/libs/), and + // because the aux classloader is parent-first, the older transitive wins + // on request deserialization (e.g. dropping fields like `check_declared`). + exclude(group = "org.lance", module = "lance-namespace-apache-client") Review Comment: Updated in 4b4442d48. -- 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]
