github-actions[bot] commented on code in PR #66805:
URL: https://github.com/apache/doris/pull/66805#discussion_r3802068191
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceStorageOptions.java:
##########
@@ -17,56 +17,171 @@
package org.apache.doris.datasource.lance;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Locale;
import java.util.Map;
+import java.util.Set;
-/** Converts normalized Doris storage properties to Lance object-store
options. */
+/**
+ * Builds the Lance object-store options for one dataset.
+ *
+ * <p>Both the FE, which opens the dataset through the Lance Java SDK, and the
BE, which opens it
+ * through lance-c, consume the map produced here, so the two cannot disagree
about how a dataset
+ * is accessed.
+ *
+ * <p>Options vended by a namespace are merged in as they arrive. The Lance
Namespace specification
+ * describes {@code storage_options} as configuration "passed directly to
Lance", so the protocol
+ * defines no key vocabulary of its own and a client cannot assume one.
Re-encoding those options
+ * into a fixed set of names would silently drop everything outside it,
including credentials
+ * spelled with a different accepted alias and every non-S3 provider's keys.
+ */
public final class LanceStorageOptions {
+ private static final Logger LOG =
LogManager.getLogger(LanceStorageOptions.class);
+
+ /**
+ * Doris backend property to Lance object-store option.
+ *
+ * <p>Lance reaches S3 through object_store, which accepts both {@code
access_key_id} and
+ * {@code aws_access_key_id}. The unprefixed spelling is chosen because it
is also the field
+ * name used by the OpenDAL backend, which performs no alias normalization
at all, so these
+ * options stay correct if that backend is ever selected.
+ */
private static final Map<String, String> S3_KEYS = new HashMap<>();
static {
- S3_KEYS.put("AWS_ACCESS_KEY", "aws_access_key_id");
- S3_KEYS.put("AWS_SECRET_KEY", "aws_secret_access_key");
- S3_KEYS.put("AWS_TOKEN", "aws_session_token");
- S3_KEYS.put("AWS_ENDPOINT", "aws_endpoint");
- S3_KEYS.put("AWS_REGION", "aws_region");
+ S3_KEYS.put("AWS_ACCESS_KEY", "access_key_id");
Review Comment:
[P1] Prevent explicit S3 options from competing with `AWS_*`
Both pinned Lance readers call `with_env_s3()` before building the store.
That method suppresses an environment value only when the raw map contains
`AmazonS3ConfigKey::as_ref()` (for example `aws_access_key_id`), but this
change emits `access_key_id`. With `AWS_ACCESS_KEY_ID` set, both spellings
remain, and `as_s3_options()` parses them to the same enum key while collecting
from a randomized HashMap, so the environment or explicit value can win
independently for access key, secret, endpoint, region, and path style. This
can mix credentials and make FE planning or BE scans authenticate against the
wrong store. Please make explicit values suppress every equivalent environment
alias (canonical `aws_*` output works with both pinned OpenDAL configs) and add
conflicting-environment boundary coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceStorageOptions.java:
##########
@@ -17,56 +17,171 @@
package org.apache.doris.datasource.lance;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Locale;
import java.util.Map;
+import java.util.Set;
-/** Converts normalized Doris storage properties to Lance object-store
options. */
+/**
+ * Builds the Lance object-store options for one dataset.
+ *
+ * <p>Both the FE, which opens the dataset through the Lance Java SDK, and the
BE, which opens it
+ * through lance-c, consume the map produced here, so the two cannot disagree
about how a dataset
+ * is accessed.
+ *
+ * <p>Options vended by a namespace are merged in as they arrive. The Lance
Namespace specification
+ * describes {@code storage_options} as configuration "passed directly to
Lance", so the protocol
+ * defines no key vocabulary of its own and a client cannot assume one.
Re-encoding those options
+ * into a fixed set of names would silently drop everything outside it,
including credentials
+ * spelled with a different accepted alias and every non-S3 provider's keys.
+ */
public final class LanceStorageOptions {
+ private static final Logger LOG =
LogManager.getLogger(LanceStorageOptions.class);
+
+ /**
+ * Doris backend property to Lance object-store option.
+ *
+ * <p>Lance reaches S3 through object_store, which accepts both {@code
access_key_id} and
+ * {@code aws_access_key_id}. The unprefixed spelling is chosen because it
is also the field
+ * name used by the OpenDAL backend, which performs no alias normalization
at all, so these
+ * options stay correct if that backend is ever selected.
+ */
private static final Map<String, String> S3_KEYS = new HashMap<>();
static {
- S3_KEYS.put("AWS_ACCESS_KEY", "aws_access_key_id");
- S3_KEYS.put("AWS_SECRET_KEY", "aws_secret_access_key");
- S3_KEYS.put("AWS_TOKEN", "aws_session_token");
- S3_KEYS.put("AWS_ENDPOINT", "aws_endpoint");
- S3_KEYS.put("AWS_REGION", "aws_region");
+ S3_KEYS.put("AWS_ACCESS_KEY", "access_key_id");
+ S3_KEYS.put("AWS_SECRET_KEY", "secret_access_key");
+ S3_KEYS.put("AWS_TOKEN", "session_token");
+ S3_KEYS.put("AWS_ENDPOINT", "endpoint");
+ S3_KEYS.put("AWS_REGION", "region");
}
+ /**
+ * Every spelling object_store accepts for the options above, mapped to
the one this class emits.
+ *
+ * <p>object_store resolves an alias and its canonical name to one config
key and keeps only one
+ * of the two values, chosen by hash order. So a namespace vending {@code
endpoint_url} while the
+ * catalog contributes {@code endpoint} does not override it - the two
survive as separate
+ * entries, and the FE and the BE can each end up using a different one.
Every accepted alias has
+ * to be recognized here, or that race simply moves to the spellings this
table misses.
+ */
+ private static final Map<String, String> CANONICAL_BY_ALIAS =
ImmutableMap.<String, String>builder()
+ .put("access_key_id", "access_key_id")
+ .put("aws_access_key_id", "access_key_id")
+ .put("secret_access_key", "secret_access_key")
+ .put("aws_secret_access_key", "secret_access_key")
+ .put("session_token", "session_token")
+ .put("aws_session_token", "session_token")
+ .put("aws_token", "session_token")
+ .put("token", "session_token")
+ .put("endpoint", "endpoint")
+ .put("endpoint_url", "endpoint")
+ .put("aws_endpoint", "endpoint")
+ .put("aws_endpoint_url", "endpoint")
Review Comment:
[P1] Honor the S3-specific endpoint before deriving HTTP
`aws_endpoint_url_s3` is also accepted by the pinned object_store, and it
takes precedence over the generic `endpoint`. Because this alias is not
recognized here, a namespace vending `aws_endpoint_url_s3=http://minio:9000`
leaves the catalog endpoint in the map and `withDerivedAllowHttp()` inspects
only that losing value. Both the FE metadata open and BE scan then select the
HTTP S3 endpoint with `allow_http` absent and fail their first request; OpenDAL
can instead ignore the key and use a different endpoint. Please resolve this
spelling with explicit S3-specific precedence into the common endpoint before
deriving/retracting `allow_http`, and cover conflicting generic/S3-specific
values.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -894,36 +894,22 @@ Status
LanceTableReader::_fill_block_from_arrow(LanceBatch* batch, Block* block,
return Status::OK();
}
+// The FE sends these already in Lance's own vocabulary, merged from the
catalog properties and
+// from whatever the namespace vended. Re-encoding them here would drop every
option this list did
+// not anticipate, so they are handed to lance-c as they arrive.
std::vector<std::string> LanceTableReader::_storage_options(
const TFileScanRangeParams* scan_params) {
- if (scan_params == nullptr || !scan_params->__isset.properties) {
+ if (scan_params == nullptr || !scan_params->__isset.lance_storage_options)
{
return {};
}
- static constexpr std::array<std::pair<std::string_view, std::string_view>,
5> kStorageKeys = {
- {{"AWS_ACCESS_KEY", "aws_access_key_id"},
- {"AWS_SECRET_KEY", "aws_secret_access_key"},
- {"AWS_TOKEN", "aws_session_token"},
- {"AWS_ENDPOINT", "aws_endpoint"},
- {"AWS_REGION", "aws_region"}}};
std::vector<std::string> options;
- options.reserve(kStorageKeys.size() * 2);
- for (const auto& [doris_key, lance_key] : kStorageKeys) {
- const auto it = scan_params->properties.find(std::string(doris_key));
- if (it != scan_params->properties.end() && !it->second.empty()) {
- options.emplace_back(lance_key);
- options.emplace_back(it->second);
- }
- }
- const auto endpoint = scan_params->properties.find("AWS_ENDPOINT");
- if (endpoint != scan_params->properties.end() &&
endpoint->second.rfind("http://";, 0) == 0) {
- options.emplace_back("allow_http");
- options.emplace_back("true");
- }
- const auto path_style = scan_params->properties.find("use_path_style");
- if (path_style != scan_params->properties.end() &&
!path_style->second.empty()) {
- const bool use_path_style = path_style->second == "true" ||
path_style->second == "1";
- options.emplace_back("aws_virtual_hosted_style_request");
- options.emplace_back(use_path_style ? "false" : "true");
+ options.reserve(scan_params->lance_storage_options.size() * 2);
+ for (const auto& [key, value] : scan_params->lance_storage_options) {
+ if (value.empty()) {
+ continue;
+ }
+ options.emplace_back(key);
Review Comment:
[P1] Reject embedded NULs before the lance-c boundary
The new opaque path preserves an escaped NUL from REST JSON through Java and
Thrift, but these strings are later passed to lance-c with `c_str()`, whose
`CStr::from_ptr` parser stops at the first NUL. A vended
`endpoint\u0000ignored` is therefore unknown to the FE merge while the BE sees
`endpoint`; likewise `bucket\u0000ignored` bypasses the FE protected-key check
and reaches lance-c as `bucket`, so metadata and scan execution can use
different locations. Please reject NUL in every option key/value while merging
and validate defensively here before constructing the C pointer array, with a
boundary test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceStorageOptions.java:
##########
@@ -17,56 +17,171 @@
package org.apache.doris.datasource.lance;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Locale;
import java.util.Map;
+import java.util.Set;
-/** Converts normalized Doris storage properties to Lance object-store
options. */
+/**
+ * Builds the Lance object-store options for one dataset.
+ *
+ * <p>Both the FE, which opens the dataset through the Lance Java SDK, and the
BE, which opens it
+ * through lance-c, consume the map produced here, so the two cannot disagree
about how a dataset
+ * is accessed.
+ *
+ * <p>Options vended by a namespace are merged in as they arrive. The Lance
Namespace specification
+ * describes {@code storage_options} as configuration "passed directly to
Lance", so the protocol
+ * defines no key vocabulary of its own and a client cannot assume one.
Re-encoding those options
+ * into a fixed set of names would silently drop everything outside it,
including credentials
+ * spelled with a different accepted alias and every non-S3 provider's keys.
+ */
public final class LanceStorageOptions {
+ private static final Logger LOG =
LogManager.getLogger(LanceStorageOptions.class);
+
+ /**
+ * Doris backend property to Lance object-store option.
+ *
+ * <p>Lance reaches S3 through object_store, which accepts both {@code
access_key_id} and
+ * {@code aws_access_key_id}. The unprefixed spelling is chosen because it
is also the field
+ * name used by the OpenDAL backend, which performs no alias normalization
at all, so these
+ * options stay correct if that backend is ever selected.
+ */
private static final Map<String, String> S3_KEYS = new HashMap<>();
static {
- S3_KEYS.put("AWS_ACCESS_KEY", "aws_access_key_id");
- S3_KEYS.put("AWS_SECRET_KEY", "aws_secret_access_key");
- S3_KEYS.put("AWS_TOKEN", "aws_session_token");
- S3_KEYS.put("AWS_ENDPOINT", "aws_endpoint");
- S3_KEYS.put("AWS_REGION", "aws_region");
+ S3_KEYS.put("AWS_ACCESS_KEY", "access_key_id");
+ S3_KEYS.put("AWS_SECRET_KEY", "secret_access_key");
+ S3_KEYS.put("AWS_TOKEN", "session_token");
+ S3_KEYS.put("AWS_ENDPOINT", "endpoint");
+ S3_KEYS.put("AWS_REGION", "region");
}
+ /**
+ * Every spelling object_store accepts for the options above, mapped to
the one this class emits.
+ *
+ * <p>object_store resolves an alias and its canonical name to one config
key and keeps only one
+ * of the two values, chosen by hash order. So a namespace vending {@code
endpoint_url} while the
+ * catalog contributes {@code endpoint} does not override it - the two
survive as separate
+ * entries, and the FE and the BE can each end up using a different one.
Every accepted alias has
+ * to be recognized here, or that race simply moves to the spellings this
table misses.
+ */
+ private static final Map<String, String> CANONICAL_BY_ALIAS =
ImmutableMap.<String, String>builder()
+ .put("access_key_id", "access_key_id")
+ .put("aws_access_key_id", "access_key_id")
+ .put("secret_access_key", "secret_access_key")
+ .put("aws_secret_access_key", "secret_access_key")
+ .put("session_token", "session_token")
+ .put("aws_session_token", "session_token")
+ .put("aws_token", "session_token")
+ .put("token", "session_token")
+ .put("endpoint", "endpoint")
+ .put("endpoint_url", "endpoint")
+ .put("aws_endpoint", "endpoint")
+ .put("aws_endpoint_url", "endpoint")
+ .put("region", "region")
+ .put("aws_region", "region")
+ .put("virtual_hosted_style_request",
"virtual_hosted_style_request")
+ .put("aws_virtual_hosted_style_request",
"virtual_hosted_style_request")
Review Comment:
[P1] Collapse OpenDAL's virtual-host canonical spelling
The alias set also needs OpenDAL's canonical `enable_virtual_host_style`. A
catalog emits `virtual_hosted_style_request` from its normalized
`use_path_style`, while a namespace vending `use_opendal=true` and
`enable_virtual_host_style=false` leaves both raw keys in the merged map. Both
pinned OpenDAL S3 configs deserialize those spellings as the same field, so
operator construction rejects a duplicate field before metadata loading. Please
map `enable_virtual_host_style` into the shared `virtual_hosted_style_request`
key and add a merge/OpenDAL regression for this input.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceStorageOptions.java:
##########
@@ -17,56 +17,171 @@
package org.apache.doris.datasource.lance;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Locale;
import java.util.Map;
+import java.util.Set;
-/** Converts normalized Doris storage properties to Lance object-store
options. */
+/**
+ * Builds the Lance object-store options for one dataset.
+ *
+ * <p>Both the FE, which opens the dataset through the Lance Java SDK, and the
BE, which opens it
+ * through lance-c, consume the map produced here, so the two cannot disagree
about how a dataset
+ * is accessed.
+ *
+ * <p>Options vended by a namespace are merged in as they arrive. The Lance
Namespace specification
+ * describes {@code storage_options} as configuration "passed directly to
Lance", so the protocol
+ * defines no key vocabulary of its own and a client cannot assume one.
Re-encoding those options
+ * into a fixed set of names would silently drop everything outside it,
including credentials
+ * spelled with a different accepted alias and every non-S3 provider's keys.
+ */
public final class LanceStorageOptions {
+ private static final Logger LOG =
LogManager.getLogger(LanceStorageOptions.class);
+
+ /**
+ * Doris backend property to Lance object-store option.
+ *
+ * <p>Lance reaches S3 through object_store, which accepts both {@code
access_key_id} and
+ * {@code aws_access_key_id}. The unprefixed spelling is chosen because it
is also the field
+ * name used by the OpenDAL backend, which performs no alias normalization
at all, so these
+ * options stay correct if that backend is ever selected.
+ */
private static final Map<String, String> S3_KEYS = new HashMap<>();
static {
- S3_KEYS.put("AWS_ACCESS_KEY", "aws_access_key_id");
- S3_KEYS.put("AWS_SECRET_KEY", "aws_secret_access_key");
- S3_KEYS.put("AWS_TOKEN", "aws_session_token");
- S3_KEYS.put("AWS_ENDPOINT", "aws_endpoint");
- S3_KEYS.put("AWS_REGION", "aws_region");
+ S3_KEYS.put("AWS_ACCESS_KEY", "access_key_id");
+ S3_KEYS.put("AWS_SECRET_KEY", "secret_access_key");
+ S3_KEYS.put("AWS_TOKEN", "session_token");
+ S3_KEYS.put("AWS_ENDPOINT", "endpoint");
+ S3_KEYS.put("AWS_REGION", "region");
}
+ /**
+ * Every spelling object_store accepts for the options above, mapped to
the one this class emits.
+ *
+ * <p>object_store resolves an alias and its canonical name to one config
key and keeps only one
+ * of the two values, chosen by hash order. So a namespace vending {@code
endpoint_url} while the
+ * catalog contributes {@code endpoint} does not override it - the two
survive as separate
+ * entries, and the FE and the BE can each end up using a different one.
Every accepted alias has
+ * to be recognized here, or that race simply moves to the spellings this
table misses.
+ */
+ private static final Map<String, String> CANONICAL_BY_ALIAS =
ImmutableMap.<String, String>builder()
+ .put("access_key_id", "access_key_id")
+ .put("aws_access_key_id", "access_key_id")
+ .put("secret_access_key", "secret_access_key")
+ .put("aws_secret_access_key", "secret_access_key")
+ .put("session_token", "session_token")
+ .put("aws_session_token", "session_token")
+ .put("aws_token", "session_token")
+ .put("token", "session_token")
+ .put("endpoint", "endpoint")
+ .put("endpoint_url", "endpoint")
+ .put("aws_endpoint", "endpoint")
+ .put("aws_endpoint_url", "endpoint")
+ .put("region", "region")
+ .put("aws_region", "region")
+ .put("virtual_hosted_style_request",
"virtual_hosted_style_request")
+ .put("aws_virtual_hosted_style_request",
"virtual_hosted_style_request")
+ .put("allow_http", "allow_http")
+ .put("aws_allow_http", "allow_http")
+ .build();
+
+ /**
+ * Aliases that supersede the catalog's value but keep the spelling the
namespace used.
+ *
+ * <p>{@code token} means an S3 session token to object_store's S3 parser
but a bearer token to
+ * its Azure one, and this class does not know which provider a dataset
uses. Renaming it would
+ * corrupt the Azure reading, so it is only used to decide which catalog
entry it replaces.
+ */
+ private static final Set<String> AMBIGUOUS_ALIASES =
ImmutableSet.of("token");
+
+ /**
+ * Options a namespace may not override, because they decide which data is
read rather than how
+ * it is accessed. Lance protects the same keys in the options it accepts
from a namespace.
+ */
+ private static final Set<String> PROTECTED_KEYS = ImmutableSet.of(
+ "bucket", "aws_bucket", "aws_bucket_name", "bucket_name", "root");
+
private LanceStorageOptions() {
}
- public static Map<String, String> forJavaSdk(Map<String, String>
backendProperties) {
+ /** Converts normalized Doris storage properties to Lance object-store
options. */
+ public static Map<String, String> toLanceOptions(Map<String, String>
backendProperties) {
Map<String, String> result = new HashMap<>();
S3_KEYS.forEach((dorisKey, lanceKey) -> putIfNotEmpty(result, lanceKey,
backendProperties.get(dorisKey)));
- String endpoint = backendProperties.get("AWS_ENDPOINT");
- if (endpoint != null && endpoint.startsWith("http://";)) {
- result.put("allow_http", "true");
- }
String usePathStyle = backendProperties.get("use_path_style");
if (usePathStyle != null && !usePathStyle.isEmpty()) {
- result.put("aws_virtual_hosted_style_request",
+ result.put("virtual_hosted_style_request",
String.valueOf(!Boolean.parseBoolean(usePathStyle)));
}
- return result;
+ return withDerivedAllowHttp(result);
}
- /** Merge Lance storage options returned by a namespace into properties
understood by Doris BE. */
- public static Map<String, String> forBackend(Map<String, String>
staticBackendProperties,
- Map<String, String> lanceStorageOptions) {
- Map<String, String> result = new HashMap<>(staticBackendProperties);
- if (lanceStorageOptions == null || lanceStorageOptions.isEmpty()) {
+ /**
+ * Merges the options a namespace vended for one table over the catalog's
own options.
+ *
+ * <p>Options a namespace may not override are dropped; everything else
replaces the catalog
+ * value, since the namespace decides how the table it just described is
reached.
+ */
+ public static Map<String, String> mergeVended(Map<String, String>
lanceOptions,
+ Map<String, String> vendedOptions) {
+ Map<String, String> result = new HashMap<>(lanceOptions);
+ if (vendedOptions == null || vendedOptions.isEmpty()) {
return result;
}
- S3_KEYS.forEach((dorisKey, lanceKey) -> putIfNotEmpty(result, dorisKey,
- lanceStorageOptions.get(lanceKey)));
- String virtualHostedStyle =
lanceStorageOptions.get("aws_virtual_hosted_style_request");
- if (virtualHostedStyle != null && !virtualHostedStyle.isEmpty()) {
- result.put("use_path_style",
String.valueOf(!Boolean.parseBoolean(virtualHostedStyle)));
+ Map<String, String> accepted = new HashMap<>();
+ Set<String> superseded = new HashSet<>();
+ vendedOptions.forEach((key, value) -> {
+ if (key == null || value == null || value.isEmpty()) {
+ return;
+ }
+ String lowerCased = key.toLowerCase(Locale.ROOT);
+ if (PROTECTED_KEYS.contains(lowerCased)) {
+ LOG.warn("Ignoring Lance storage option '{}' vended by the
namespace because it "
+ + "would change which data is read", key);
+ return;
+ }
+ String canonical = CANONICAL_BY_ALIAS.get(lowerCased);
+ if (canonical != null) {
+ superseded.add(canonical);
+ }
+ accepted.put(canonical != null &&
!AMBIGUOUS_ALIASES.contains(lowerCased)
Review Comment:
[P1] Keep opaque options within the BE provider vocabulary
Using one map still does not make the readers interpret it identically:
Lance Java 9.1.0-beta.3 pins OpenDAL 0.57, while lance-c 0.1.6 pins 0.56. For a
public S3 table vending `use_opendal=true` and `skip_signature=true`, the FE
recognizes the 0.57 option and opens the manifest unsigned, but the BE's 0.56
backend ignores it and attempts signing without credentials, so planning
succeeds and every scan fails. Please align the provider versions or
normalize/reject options to their common vocabulary before promising identical
FE/BE behavior, and add a cross-boundary regression for this input.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
