github-actions[bot] commented on code in PR #69385:
URL: https://github.com/apache/doris/pull/69385#discussion_r4335152775
##########
fe/fe-filesystem/fe-filesystem-spi/src/main/java/org/apache/doris/filesystem/spi/S3CompatibleFileSystem.java:
##########
@@ -1140,6 +1178,53 @@ public static Pattern compileGlobPattern(String
keyPattern) {
return
Pattern.compile(globToRegex(GlobPattern.expandNumericRanges(keyPattern)));
}
+ private static List<String> expandDeterministicKeyPattern(String pattern,
int maxPaths) {
+ // Keep the original deterministic-path boundary, including escaped
glob literals.
+ if (maxPaths <= 0 || pattern.isEmpty() ||
pattern.endsWith(DIR_MARKER_SUFFIX)
+ || pattern.indexOf('*') >= 0 || pattern.indexOf('?') >= 0 ||
pattern.indexOf('\\') >= 0) {
+ return null;
+ }
+ List<String> expanded = expandGlobListPrefixes(pattern, false,
maxPaths);
+ if (expanded == null || expanded.stream().anyMatch(key ->
key.isEmpty() || key.endsWith(DIR_MARKER_SUFFIX))) {
+ return null;
+ }
+ List<String> keys = new ArrayList<>(new LinkedHashSet<>(expanded));
+ keys.sort(UTF8_BINARY_ORDER);
+ return keys;
+ }
+
+ private RemoteObject headObjectIfPresent(String path) throws IOException {
+ try {
+ return objStorage.headObject(path);
+ } catch (IOException e) {
+ if (isNotFoundError(e)) {
+ return null;
+ }
+ throw e;
+ }
+ }
+
+ private GlobListing globListDeterministicKeys(String bucket, String
patternPrefix, String base,
+ List<String> keys) throws IOException {
+ List<FileEntry> files = new ArrayList<>();
+ String lastMatchedKey = "";
+ for (String key : keys) {
+ RemoteObject obj = headObjectIfPresent(base + key);
+ if (obj == null) {
+ continue;
+ }
+ String objectKey = obj.getKey();
Review Comment:
[P2] Preserve the glob matcher when emitting HEAD results
The prefix expander can produce a superset of the keys accepted by the
existing matcher. For `s3://bucket/dir/file[ab&&bc].csv`, `globToRegex`
preserves [Java's character-class
intersection](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/regex/Pattern.html)
(only `fileb.csv` matches), but expansion produces `filea.csv`, `fileb.csv`,
`file&.csv` and `filec.csv`. This helper returns every successful HEAD without
applying the matcher, so `BrokerLoadPendingTask` (null cursor, zero limits) can
now import a/c files that LIST excluded. Fall back for unsupported class syntax
or validate expanded/returned keys against the same matcher before emitting
entries.
##########
fe/fe-core/src/main/java/org/apache/doris/fs/FileSystemFactory.java:
##########
@@ -165,13 +184,23 @@ public static
List<org.apache.doris.filesystem.properties.StorageProperties> bin
*/
public static org.apache.doris.filesystem.FileSystem getFileSystem(
FileSystemProperties spiProperties) throws IOException {
+ return getFileSystemWithEffectiveProperties(spiProperties,
withRuntimeFileSystemProperties(
+ spiProperties.rawProperties(),
+ Config.s3_skip_list_for_deterministic_path,
Config.s3_head_request_max_paths));
+ }
+
+ static org.apache.doris.filesystem.FileSystem
getFileSystemWithEffectiveProperties(
+ FileSystemProperties spiProperties, Map<String, String>
effectiveProperties) throws IOException {
String providerName = spiProperties.providerName();
FileSystemPluginManager mgr = pluginManager;
List<FileSystemProvider> providers = mgr != null ? mgr.getProviders()
: getProviders();
for (FileSystemProvider provider : providers) {
if (provider.name().equalsIgnoreCase(providerName)) {
try {
- return provider.createUntyped(spiProperties);
+ // Rebind through the original provider so runtime
controls do not change routing.
+ FileSystemProperties effective = spiProperties instanceof
S3CompatibleFileSystemProperties
+ ? provider.bind(effectiveProperties) :
spiProperties;
Review Comment:
[P2] Preserve case-insensitive properties when rebinding
`S3SourceOffsetProvider.getNextOffset()` and `fetchRemoteMeta()`
intentionally bind a `TreeMap(String.CASE_INSENSITIVE_ORDER)` before creating
the filesystem. With `fs.s3.support=true`, a private `s3.endpoint`, and
`S3.REGION=us-east-1`, the initial binding contains a usable region. Its
`rawProperties()` snapshot and this runtime overlay are `HashMap`s, however, so
the new `provider.bind(effectiveProperties)` ignores `S3.REGION` and throws
`Region is not set`. Those metadata/offset calls previously created the
filesystem from the bound region; they now fail before LIST, even for positive
limits or disabled HEAD. Preserve the lookup semantics or apply runtime
controls without discarding bound values.
##########
fe/fe-filesystem/fe-filesystem-spi/src/main/java/org/apache/doris/filesystem/spi/S3CompatibleFileSystem.java:
##########
@@ -1151,11 +1236,28 @@ public GlobListing globListWithLimit(Location path,
String startAfter, long maxB
String base = uriBase(uri, parsed);
String expandedKeyPattern =
GlobPattern.expandNumericRanges(keyPattern);
+ Pattern matcher = Pattern.compile(globToRegex(expandedKeyPattern));
+ // Preserve #60414's eligibility: path-style addressing and
incremental/limited reads
+ // must keep LIST semantics. Even an empty, non-null cursor selects
the LIST path.
+ if (skipListForDeterministicPath && !usePathStyle
+ && maxBytes <= 0 && maxFiles <= 0 && startAfter == null) {
Review Comment:
[P1] Route initial TVF listing requests through the HEAD fast path
`ExternalFileTableValuedFunction.parseFile()` still calls
`globListWithLimit(Location.of(path), "", 0, 0)` (line 190), so `startAfter ==
null` excludes every normal `S3(...)` TVF call, including the regression added
here. With object reads/HEAD allowed and ListBucket denied, exact and brace
paths still execute LIST and fail with the same 403 this PR is intended to fix.
Keep cursor-based LIST semantics, but pass null from the initial TVF call and
cover that production call chain.
--
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]
