FANNG1 commented on code in PR #66805:
URL: https://github.com/apache/doris/pull/66805#discussion_r3822267330
##########
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:
Follow-up: the fix I described above was later reworked, so this thread no
longer matches the code.
Emitting the canonical `aws_*` spelling for *vended* options turned out to
be wrong for every non-S3 backend. object_store's Azure parser reads `endpoint`
but not `aws_endpoint` (`azure/builder.rs:448`), and Lance's OSS provider
requires `endpoint` and reads `access_key_id` (`oss.rs:71-73`, `:87`) — so
canonicalizing a namespace's options onto the S3 names lost the Azure endpoint
outright and made OSS fail. That was a regression I introduced here, fixed in
a04c8e1.
What survives is the half this comment was actually about. The catalog's own
configuration is still emitted under the canonical spellings, and vended
options are still resolved onto them — but only for a dataset whose URL scheme
routes to object_store's AWS provider, where the spelling is known to be right.
`LanceStorageProvider` picks that the way lance-io's registry does; everything
else passes through untouched. 88f5053.
The environment-suppression reasoning holds as stated, and the `allow_http`
caveat with it.
##########
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:
Follow-up: this one took three passes, not two, and the endpoint
double-write I described above is gone.
Writing the resolved endpoint under both `aws_endpoint` and
`aws_endpoint_url_s3` was part of a broader canonicalization of vended options,
which I had to remove: it destroyed working configuration for Azure and OSS,
whose providers read `endpoint` and not the S3 spellings. a04c8e1.
The original observation still stands, and the current code handles it by
leaving the key alone rather than folding it in. object_store parses
`aws_endpoint_url_s3` into a config key of its own and prefers it (`let
endpoint = self.s3_endpoint.or(self.endpoint)`), so a namespace that vends it
already wins deterministically — no rewriting needed. Folding it onto the
generic entry, as I did in ed17099, was the actual mistake: it replaced a
defined precedence with map iteration order.
The environment gap I claimed to close in e850bff is therefore open again
for that one key: an `AWS_ENDPOINT_URL_S3` in the FE or BE process is not
suppressed. Closing it would mean emitting a spelling Doris never uses, which
is the kind of thing this path is now trying not to do; it is recorded under
"Not in scope" instead.
##########
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:
Follow-up: still fixed, but now scoped to datasets that actually use the
OpenDAL S3 backend.
`enable_virtual_host_style` is resolved onto the same entry as the two
object_store spellings, exactly as described — but only when the dataset URL
routes to object_store's AWS provider (`s3`, `s3+ddb`). For an `oss://` or
`az://` dataset the option is passed through untouched, because those providers
read different names and rewriting theirs is what broke them in the first
iteration of this fix. a04c8e1, 88f5053.
##########
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:
Follow-up: the assessment above is unchanged, and the residual skew is now
recorded in the PR description under "Not in scope" rather than only in a code
comment.
One thing did change in this comment's favour. The concern was that one
option map cannot mean the same thing to two differently-pinned readers. The
code no longer tries to make it mean the same thing by construction: options
are resolved only for the provider Lance will route the dataset to, and
everything else is passed through for Lance to interpret. That does not close
the OpenDAL 0.56/0.57 difference — nothing here can — but it stops the FE from
asserting an interpretation the BE may not share.
##########
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:
Follow-up: the handling is stricter than what I described here.
Dropping the option turned out to be the wrong response: an FE that drops it
and a BE that does not disagree in exactly the way the check exists to prevent.
Both ends now fail instead — the FE rejects the option, and `_storage_options`
returns `InvalidArgument` rather than warning and continuing, which also
matches the convention elsewhere in this file. a04c8e1.
The two qualifications above still stand: the severity assumes a hostile
namespace server, which is not a boundary this check defends, and the `bucket`
example does not hold because both providers re-derive it from the dataset URL.
--
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]
