Copilot commented on code in PR #55498:
URL: https://github.com/apache/doris/pull/55498#discussion_r2311734511
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -7848,6 +7848,8 @@ public MysqlDataDescription
visitMysqlDataDesc(DorisParser.MysqlDataDescContext
properties = ImmutableMap.of();
}
+ // MySQL load only support csv, set it explicitly
+ properties.put("format", "csv");
Review Comment:
The code is calling `put()` on an `ImmutableMap` which will throw an
`UnsupportedOperationException`. Since `properties` is created as
`ImmutableMap.of()` on line 7848, it cannot be modified. Consider creating a
mutable map or using a builder pattern.
##########
fe/fe-core/src/main/java/org/apache/doris/load/BrokerFileGroup.java:
##########
@@ -285,7 +293,17 @@ public void setFileSize(List<Long> fileSize) {
this.fileSize = fileSize;
}
+ public void
initDeferredFileFormatPropertiesIfNecessary(List<TBrokerFileStatus>
fileStatuses) {
+ if (fileFormatProperties instanceof DeferredFileFormatProperties) {
+ Preconditions.checkState(fileStatuses != null &&
!fileStatuses.isEmpty());
+ TBrokerFileStatus fileStatus = fileStatuses.get(0);
+ TFileFormatType formatType =
Util.getFileFormatTypeFromPath(fileStatus.path);
+ ((DeferredFileFormatProperties)
fileFormatProperties).deferInit(formatType);
+ }
+ }
+
public boolean isBinaryFileFormat() {
+ // Must call initDeferredFileFormatPropertiesIfNecessary before
Review Comment:
The method assumes `initDeferredFileFormatPropertiesIfNecessary` has been
called but doesn't verify it. If `fileFormatProperties` is still a
`DeferredFileFormatProperties` instance that hasn't been initialized, this will
incorrectly return `false`. Consider checking if it's a
`DeferredFileFormatProperties` and either throw an exception or delegate to the
actual implementation.
```suggestion
// Defensive: check for uninitialized DeferredFileFormatProperties
if (fileFormatProperties instanceof DeferredFileFormatProperties) {
throw new IllegalStateException("DeferredFileFormatProperties
must be initialized before calling isBinaryFileFormat()");
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/fileformat/DeferredFileFormatProperties.java:
##########
@@ -0,0 +1,117 @@
+// 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.doris.datasource.property.fileformat;
+
+import org.apache.doris.nereids.exceptions.AnalysisException;
+import org.apache.doris.thrift.TFileAttributes;
+import org.apache.doris.thrift.TFileCompressType;
+import org.apache.doris.thrift.TFileFormatType;
+import org.apache.doris.thrift.TResultFileSinkOptions;
+
+import com.google.common.base.Preconditions;
+
+import java.util.Map;
+
+/**
+ * A wrapper of FileFormatProperties, which defers the initialization of the
actual FileFormatProperties.
+ * Currently, this is only used in broker load.
+ * In broker load, user may not specify the file format properties, and we can
not even infer the file format
+ * from path at the beginning, because the path may be a directory with
wildcard.
+ * So we can only get the file suffix after listing files, and then initialize
the actual.
+ *
+ * When using this class, you must call {@link #deferInit(TFileFormatType)}
after getting the actual file format type
+ * before using other methods.
+ * And all methods must override and delegate to the actual
FileFormatProperties.
+ */
+public class DeferredFileFormatProperties extends FileFormatProperties {
+
+ private FileFormatProperties delegate;
+ private Map<String, String> origProperties;
+
+ public DeferredFileFormatProperties() {
+ super(null, null);
+ }
+
+ @Override
+ public void analyzeFileFormatProperties(Map<String, String>
formatProperties, boolean isRemoveOriginProperty)
+ throws AnalysisException {
+ this.origProperties = formatProperties;
+ }
+
+ @Override
+ public void fullTResultFileSinkOptions(TResultFileSinkOptions sinkOptions)
{
+ Preconditions.checkNotNull(delegate);
+ delegate.fullTResultFileSinkOptions(sinkOptions);
+ }
+
+ @Override
+ public TFileAttributes toTFileAttributes() {
+ Preconditions.checkNotNull(delegate);
+ return delegate.toTFileAttributes();
+ }
+
+ @Override
+ protected String getOrDefault(Map<String, String> props, String key,
String defaultValue,
+ boolean isRemove) {
+ Preconditions.checkNotNull(delegate);
+ return delegate.getOrDefault(props, key, defaultValue, isRemove);
+ }
+
+ public TFileFormatType getFileFormatType() {
+ Preconditions.checkNotNull(delegate);
+ return delegate.getFileFormatType();
+ }
+
+ public TFileCompressType getCompressionType() {
+ Preconditions.checkNotNull(delegate);
+ return delegate.getCompressionType();
+ }
+
+ public String getFormatName() {
+ Preconditions.checkNotNull(delegate);
+ return delegate.getFormatName();
+ }
+
+ public FileFormatProperties getDelegate() {
+ Preconditions.checkNotNull(delegate);
+ return delegate;
+ }
+
+ public void deferInit(TFileFormatType formatType) {
+ switch (formatType) {
+ case FORMAT_PARQUET: {
+ this.formatName = FORMAT_PARQUET;
+ break;
+ }
+ case FORMAT_ORC: {
+ this.formatName = FORMAT_ORC;
+ break;
+ }
+ case FORMAT_JSON: {
+ this.formatName = FORMAT_JSON;
+ break;
+ }
+ default: {
+ this.formatName = FORMAT_CSV;
+ break;
+ }
+ }
+ delegate =
FileFormatProperties.createFileFormatProperties(this.formatName);
+ delegate.analyzeFileFormatProperties(origProperties, false);
Review Comment:
The `createFileFormatProperties` method can throw an `AnalysisException`,
but `deferInit` doesn't declare this exception in its signature. This will
cause a compilation error. The method signature should include `throws
AnalysisException`.
--
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]
