vinishjail97 commented on code in PR #830:
URL: https://github.com/apache/incubator-xtable/pull/830#discussion_r3439774049
##########
xtable-utilities/src/main/java/org/apache/xtable/utilities/RunSync.java:
##########
@@ -214,18 +215,30 @@ static CatalogConfig getIcebergCatalogConfig(String
icebergCatalogConfigPath) th
static ConversionSourceProvider<?> getConversionSourceProvider(
String conversionProviderConfigpath, DatasetConfig datasetConfig,
Configuration hadoopConf)
throws IOException {
- // Process source format
String sourceFormat = datasetConfig.sourceFormat;
byte[] customConfig =
getCustomConfigurations(conversionProviderConfigpath);
TableFormatConverters tableFormatConverters =
loadTableFormatConversionConfigs(customConfig);
+ if (sourceFormat == null
+ && datasetConfig.getDatasets() != null
+ && !datasetConfig.getDatasets().isEmpty()) {
+ DatasetConfig.Table firstTable = datasetConfig.getDatasets().get(0);
+ if (firstTable.getTableBasePath() != null) {
+ try {
+ String tablePath = firstTable.getTableBasePath();
+ sourceFormat = DetectSourceType.detectFormat(tablePath, hadoopConf);
+ log.info(
+ "Source format was omitted in config. Auto-detected table
format: {}", sourceFormat);
+ } catch (Exception e) {
+ throw new IllegalArgumentException(
Review Comment:
`detectFormat` returns `"UNKNOWN"` rather than throwing when no format dir
is found, so this catch almost never fires — the real failure falls through to
the NPE below. And when it does fire, `sourceFormat` is still null
(detectFormat threw before assigning), so the message reads `Source format null
is not supported`, which is misleading.
##########
xtable-utilities/src/main/java/org/apache/xtable/utilities/RunSync.java:
##########
@@ -214,18 +215,30 @@ static CatalogConfig getIcebergCatalogConfig(String
icebergCatalogConfigPath) th
static ConversionSourceProvider<?> getConversionSourceProvider(
String conversionProviderConfigpath, DatasetConfig datasetConfig,
Configuration hadoopConf)
throws IOException {
- // Process source format
String sourceFormat = datasetConfig.sourceFormat;
byte[] customConfig =
getCustomConfigurations(conversionProviderConfigpath);
TableFormatConverters tableFormatConverters =
loadTableFormatConversionConfigs(customConfig);
+ if (sourceFormat == null
+ && datasetConfig.getDatasets() != null
+ && !datasetConfig.getDatasets().isEmpty()) {
+ DatasetConfig.Table firstTable = datasetConfig.getDatasets().get(0);
+ if (firstTable.getTableBasePath() != null) {
+ try {
+ String tablePath = firstTable.getTableBasePath();
+ sourceFormat = DetectSourceType.detectFormat(tablePath, hadoopConf);
+ log.info(
+ "Source format was omitted in config. Auto-detected table
format: {}", sourceFormat);
+ } catch (Exception e) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Source format %s is not supported. Known source and target
formats are %s",
+ sourceFormat,
tableFormatConverters.getTableFormatConverters().keySet()));
+ }
+ }
+ }
+
TableFormatConverters.ConversionConfig sourceConversionConfig =
tableFormatConverters.getTableFormatConverters().get(sourceFormat);
- if (sourceConversionConfig == null) {
- throw new IllegalArgumentException(
- String.format(
- "Source format %s is not supported. Known source and target
formats are %s",
- sourceFormat,
tableFormatConverters.getTableFormatConverters().keySet()));
- }
String sourceProviderClass =
sourceConversionConfig.conversionSourceProviderClass;
Review Comment:
The original `if (sourceConversionConfig == null)` check was removed. When
detection returns `"UNKNOWN"` (or `sourceFormat` stays null because `datasets`
is empty/basePath is null), the map lookup at line 241 returns null and this
line NPEs. This swaps a clear `IllegalArgumentException` for a cryptic
NullPointerException — please restore the null check.
##########
xtable-api/src/main/java/org/apache/xtable/conversion/DetectSourceType.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.xtable.conversion;
+
+import java.io.IOException;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+
+public class DetectSourceType {
Review Comment:
Utility class with only static methods: the name is a verb phrase (prefer a
noun like `TableFormatDetector`), and add a private constructor to prevent
instantiation.
##########
xtable-api/src/main/java/org/apache/xtable/conversion/DetectSourceType.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.xtable.conversion;
+
+import java.io.IOException;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+
+public class DetectSourceType {
+ // helper method to detect input format
+ static String safeDetectFormat(String basePath, CatalogConfig catalogConfig)
{
+ try {
+ Configuration conf = new Configuration();
Review Comment:
This builds a fresh `new Configuration()` from only `catalogConfig` options,
whereas `RunSync` passes the fully-loaded `hadoopConf`. Two divergent detection
paths: the `SourceTable` path won't inherit S3/GCS credentials from the hadoop
config, so `fs.exists()` will silently fail and return `"UNKNOWN"`. Consider a
single entry point that takes a `Configuration`.
##########
xtable-api/src/main/java/org/apache/xtable/conversion/DetectSourceType.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.xtable.conversion;
+
+import java.io.IOException;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+
+public class DetectSourceType {
+ // helper method to detect input format
+ static String safeDetectFormat(String basePath, CatalogConfig catalogConfig)
{
+ try {
+ Configuration conf = new Configuration();
+ // map props to hadoop's
+ if (catalogConfig != null && catalogConfig.getCatalogOptions() != null) {
+ catalogConfig.getCatalogOptions().forEach(conf::set);
+ }
+ return
DetectSourceType.detectFormat(ExternalTable.sanitizeBasePath(basePath), conf);
+ } catch (IOException e) {
+ return "UNKNOWN";
Review Comment:
Silently returning `"UNKNOWN"` swallows real errors (auth failures,
malformed path). At minimum `log.warn("Failed to detect format for {}",
basePath, e)` so detection failures are diagnosable.
##########
xtable-api/src/main/java/org/apache/xtable/conversion/DetectSourceType.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.xtable.conversion;
+
+import java.io.IOException;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+
+public class DetectSourceType {
+ // helper method to detect input format
+ static String safeDetectFormat(String basePath, CatalogConfig catalogConfig)
{
+ try {
+ Configuration conf = new Configuration();
+ // map props to hadoop's
+ if (catalogConfig != null && catalogConfig.getCatalogOptions() != null) {
+ catalogConfig.getCatalogOptions().forEach(conf::set);
+ }
+ return
DetectSourceType.detectFormat(ExternalTable.sanitizeBasePath(basePath), conf);
Review Comment:
Double sanitization: this sanitizes `basePath`, then `detectFormat`
sanitizes `pathStr` again at line 43. Pass the raw path and let `detectFormat`
sanitize once.
##########
xtable-api/src/main/java/org/apache/xtable/conversion/DetectSourceType.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.xtable.conversion;
+
+import java.io.IOException;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+
+public class DetectSourceType {
+ // helper method to detect input format
+ static String safeDetectFormat(String basePath, CatalogConfig catalogConfig)
{
+ try {
+ Configuration conf = new Configuration();
+ // map props to hadoop's
+ if (catalogConfig != null && catalogConfig.getCatalogOptions() != null) {
+ catalogConfig.getCatalogOptions().forEach(conf::set);
+ }
+ return
DetectSourceType.detectFormat(ExternalTable.sanitizeBasePath(basePath), conf);
+ } catch (IOException e) {
+ return "UNKNOWN";
+ }
+ }
+
+ public static String detectFormat(String pathStr, Configuration conf) throws
IOException {
+ String sanitizeBasePath = ExternalTable.sanitizeBasePath(pathStr);
+ Path basePath = new Path(sanitizeBasePath);
+ FileSystem fs = basePath.getFileSystem(conf);
+
+ if (!fs.exists(basePath)) {
+ return "UNKNOWN";
+ }
+
+ if (fs.exists(new Path(basePath, "_delta_log"))) {
+ return "DELTA";
Review Comment:
Magic strings — reuse the existing constants in
`org.apache.xtable.model.storage.TableFormat` (`HUDI`/`ICEBERG`/`DELTA`) here
and below. Also, `"UNKNOWN"` is not a real format and is precisely what
produces the downstream NPE; prefer returning `Optional<String>`/null and
having callers handle "not detected" explicitly.
##########
xtable-utilities/src/test/java/org/apache/xtable/utilities/ITRunSync.java:
##########
@@ -93,6 +93,21 @@ void testContinuousSyncMode(@TempDir Path tempDir) throws
IOException {
}
}
+ @Test
+ void testSingleSyncModeWithoutInputTableFormat(@TempDir Path tempDir) throws
IOException {
Review Comment:
Only the Hudi happy path is covered. Add a negative test for an undetectable
/ empty base path — that's exactly the path where the removed null check now
produces an NPE. A parameterized test across DELTA/HUDI/ICEBERG would also
strengthen this.
--
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]
