github-actions[bot] commented on code in PR #69527:
URL: https://github.com/apache/doris/pull/69527#discussion_r4361485535
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/S3TableValuedFunction.java:
##########
@@ -89,6 +106,32 @@ public S3TableValuedFunction(Map<String, String>
properties, List<TBrokerFileSta
}
+ private static boolean isRemoteLanceUri(String path) {
+ try {
+ URI uri = new URI(path == null ? "" : path);
+ String scheme = uri.getScheme();
+ String authority = uri.getRawAuthority();
+ if (scheme == null ||
!LANCE_REMOTE_SCHEMES.contains(scheme.toLowerCase(Locale.ROOT))
+ || uri.isOpaque()) {
+ return false;
+ }
+ if (authority == null || authority.isEmpty()) {
+ // GooseFS can resolve its remote master from storage options
or configuration.
+ return "goosefs".equalsIgnoreCase(scheme) && uri.getRawPath()
!= null
+ && uri.getRawPath().startsWith("/");
+ }
+ // Lance falls back to a local path if Rust's URL parser rejects
the URI. Java's URI
+ // accepts registry authorities with invalid ports, which Rust
rejects. Validate the
+ // host/port separately without disallowing legitimate bucket
names with underscores.
+ String host =
HostAndPort.fromString(authority.substring(authority.lastIndexOf('@') + 1))
+ .requireBracketsForIPv6().getHost();
+ // Java accepts IPv6 zone identifiers, while Rust's URL parser
rejects them.
+ return !host.isEmpty() && !(host.contains(":") &&
host.contains("%"));
Review Comment:
[P1] Reject authorities that make Lance fall back to local paths
`S3("format"="lance", "fs.broker.support"="true",
"uri"="s3://bucket:-0/../../dataset.lance")` passes this check: Java URI
accepts the registry authority, and [HostAndPort parses `-0` as integer
zero](https://github.com/google/guava/blob/v33.2.1/guava/src/com/google/common/net/HostAndPort.java#L156-L192).
The pinned Rust URL parser [rejects the minus
sign](https://github.com/servo/rust-url/blob/v2.5.8/url/src/parser.rs#L1067-L1099),
so Lance [converts the parse failure into a local
path](https://github.com/lance-format/lance/blob/v12.0.0/rust/lance-io/src/object_store.rs#L483-L529).
`PathAbs` removes the two parent components, selecting `<process
cwd>/dataset.lance` without requiring either fake directory to exist. For an
existing dataset there, an ordinary user can open FE-local metadata while
`enable_local_tvf=false`; if the target also exists on the selected BE, its
`FILE_S3` range skips secure-root validation and reads locally too.
`s3://[::ffff:127.0.0.01]/../../dataset.lance
` is another accepted authority that Rust rejects because of its leading-zero
IPv4 tail. Validate the complete authority with SDK-compatible rules, or
prohibit downstream local fallback for remote TVFs, and cover both cases before
metadata loading.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -35,11 +35,36 @@
#include "format_v2/lance/lance_reader_helper.h"
#include "format_v2/lance/lance_runtime_filter_helper.h"
#include "format_v2/lance/lance_session_manager.h"
+#include "io/fs/local_file_system.h"
#include "runtime/file_scan_profile.h"
#include "runtime/runtime_state.h"
namespace doris::format::lance {
+namespace {
+Status resolve_dataset_uri(const TFileRangeDesc& range, const
TFileScanRangeParams& scan_params,
+ std::string* uri) {
+ const auto& requested_uri =
range.table_format_params.lance_params.dataset_uri;
+ // Modern plans tag each range; older plans put the type in the scan
parameters.
+ // Untagged catalog scans retain their original storage-provider selection.
+ const bool is_local = range.__isset.file_type
+ ? range.file_type == TFileType::FILE_LOCAL
+ : scan_params.__isset.file_type &&
+ scan_params.file_type ==
TFileType::FILE_LOCAL;
+ // LOCAL TVF uses version zero for both schema discovery and whole-dataset
scans.
+ // Catalog scans pin a positive snapshot version, including file://
catalogs whose
+ // operator-configured warehouse is independent of the LOCAL TVF secure
directory.
+ if (is_local && range.table_format_params.lance_params.version == 0) {
+ // LOCAL Lance bypasses glob because it opens a dataset directory.
Apply the same
+ // canonical secure-root check before either schema discovery or a
cached dataset open.
+ return io::global_local_filesystem()->canonicalize_local_file(
Review Comment:
[P2] Decode file URIs before validating LOCAL Lance paths
With `user_files_secure_path=/srv/user_files`, an enabled, authorized LOCAL
Lance call using `file_path=file:///srv/user_files/a%20b.lance` should open the
in-root dataset `/srv/user_files/a b.lance`. The new resolver calls
`convert_to_abs_path`, which strips the scheme/authority but leaves `%20`
literal, so canonicalization now rejects this valid dataset (or selects the
wrong one if a literal `a%20b.lance` also exists). Previously the URI reached
Lance directly, whose pinned [FileStoreProvider decodes it with
`Url::to_file_path`](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance-io/src/object_store/providers/local.rs#L135-L147).
Decode actual file URIs before the canonical containment check while
preserving literal percent characters in bare paths, and add an escaped-URI
positive case.
--
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]
