github-actions[bot] commented on code in PR #69526:
URL: https://github.com/apache/doris/pull/69526#discussion_r4366625863
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertIntoTVFCommand.java:
##########
@@ -185,6 +188,10 @@ private void deleteExistingFilesInFE(String tvfName,
Map<String, String> props)
fsCopyProps.remove("compress_type");
StorageAdapter storageAdapter = StorageAdapter.of(fsCopyProps);
+ if (storageAdapter.getType() == StorageTypeId.HDFS) {
Review Comment:
[P1] Gate the LOCAL adapter selected for FE cleanup
With `enable_local_tvf=false`, a global LOAD user can execute `INSERT INTO
s3('file_path'='/tmp/local_gate_dir/prefix_', 'format'='csv',
'fs.local.support'='true', 'delete_existing_files'='true') SELECT 1`. The
explicit support flag makes `StorageAdapter.of` select the LOCAL provider
during both sink binding and cleanup. This HDFS-only condition then skips
admission, and `FileSystemFactory.getFileSystem(storageAdapter)` preserves that
LOCAL binding: `LocalFileSystem.delete(..., true)` recursively deletes
`/tmp/local_gate_dir/` on the FE before coordinator creation or BE writer
setup. A subsequent S3 failure cannot prevent or undo the deletion. Despite its
test-only comment, the local plugin is included in normal `build.sh` packaging.
Gate `StorageTypeId.LOCAL` before filesystem creation and cover this
effective-adapter case with a no-filesystem-interaction test. This is a
separate FE cleanup path from the fixed nominal-LOCAL and JNI writer findings.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -137,6 +140,129 @@ private void getFileListFromBackend() throws
AnalysisException {
}
}
+ /** Apply LOCAL admission to the effective sink before planning or
filesystem side effects. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String>
properties) {
+ if ("local".equalsIgnoreCase(tvfName) ||
isLocalWriter(properties.getOrDefault("writer_class", ""))) {
+ checkEnabled();
+ }
+ if ("hdfs".equalsIgnoreCase(tvfName)) {
+ if (!properties.containsKey("writer_class")) {
+ checkHdfsSinkEnabled(properties);
+ }
+ if
(Boolean.parseBoolean(properties.getOrDefault("delete_existing_files",
"false"))) {
+
checkHadoopFileSystemEnabled(properties.getOrDefault("file_path", ""),
properties);
+ }
+ }
+ }
+
+ private static boolean isLocalWriter(String writer) {
+ // BE splits the complete identifier first, then passes each half
through JNI C strings.
+ int separator = writer.indexOf(':');
+ return separator > 0 &&
"java-writer".equals(nativeStringPrefix(writer.substring(0, separator)))
+ &&
"local-file".equals(nativeStringPrefix(writer.substring(separator + 1)));
+ }
+
+ /** Check the NameNode and provider used by the native FILE_HDFS writer. */
+ public static void checkHdfsSinkEnabled(Map<String, String> properties) {
+ String nameNode =
nativeStringPrefix(properties.getOrDefault("fs.defaultFS", "")).trim();
+ if ("default".equals(nameNode)) {
+ // libhdfs delegates this selector to the BE's defaults, which may
select local storage.
+ checkEnabled();
+ }
+ // libhdfs prefixes bare NameNodes with hdfs:// rather than using the
output path's scheme.
+ checkHadoopSchemeEnabled(nameNode.contains("://") ?
uriScheme(nameNode) : "hdfs", properties);
+ }
+
+ /** Check Hadoop's path/default-filesystem selection before FE-side
cleanup. */
+ public static void checkHadoopFileSystemEnabled(String path, Map<String,
String> properties) {
+ String scheme = uriScheme(path);
+ if (!scheme.isEmpty()) {
+ checkHadoopSchemeEnabled(scheme, properties);
+ return;
+ }
+ String defaultFs = hadoopSelectorProperty("fs.defaultFS", properties);
+ String legacyDefaultFs = hadoopSelectorProperty("fs.default.name",
properties);
+ // Configuration.set synchronizes aliases; either value can win when
the map is applied.
+ // Do not assume that the canonical key overrides a local legacy
selector.
+ if (defaultFs != null) {
+ checkHadoopDefaultEnabled(defaultFs, properties);
+ }
+ if (legacyDefaultFs != null) {
+ checkHadoopDefaultEnabled(legacyDefaultFs, properties);
+ }
+ if (defaultFs == null && legacyDefaultFs == null) {
+ checkHadoopSchemeEnabled("file", properties);
+ }
+ }
+
+ private static void checkHadoopDefaultEnabled(String value, Map<String,
String> properties) {
+ value = hadoopSelectorValue(value).trim();
+ // Hadoop retains the old "local" default-FS spelling.
+ String scheme = "local".equals(value) ? "file" : uriScheme(value);
+ checkHadoopSchemeEnabled(scheme.isEmpty() ? "hdfs" : scheme,
properties);
+ }
+
+ private static void checkHadoopSchemeEnabled(String scheme, Map<String,
String> properties) {
+ if ("file".equalsIgnoreCase(scheme)) {
+ checkEnabled();
+ }
+ String implementationKey = "fs." + scheme + ".impl";
Review Comment:
[P1] Check delegated ViewFS targets before admitting Hadoop sinks
With `enable_local_tvf=false`, `INSERT INTO
hdfs('file_path'='/data/local_gate_', 'format'='csv',
'fs.defaultFS'='viewfs://local/',
'fs.viewfs.mounttable.local.link./data'='file:///tmp') SELECT 1` passes both
the raw and normalized admission checks: the scheme is `viewfs` and no direct
local implementation override is needed. `HdfsProperties` preserves the mount
configuration, so `FileFactory` opens the native FILE_HDFS writer through
libhdfs and Hadoop's built-in ViewFileSystem. Its [create
implementation](https://github.com/apache/hadoop/blob/rel/release-3.4.2/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java#L445-L456)
resolves `/data` to the local mount and delegates creation, producing
`/tmp/local_gate_<query>_0.csv` on the BE with only global LOAD privilege.
Checking the top-level scheme/direct class therefore does not enforce the new
local-storage policy. Validate delegated targets, including
normalized/resource-derived ViewFS
mounts, or require opt-in when their effective storage cannot be established;
add off/on INSERT/EXPLAIN and direct-binding coverage.
--
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]
