yihua commented on code in PR #12843:
URL: https://github.com/apache/hudi/pull/12843#discussion_r1999213711
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/HoodieFileGroupReader.java:
##########
@@ -139,26 +139,26 @@ public HoodieFileGroupReader(HoodieReaderContext<T>
readerContext,
/**
* Initialize correct record buffer
*/
- private static HoodieFileGroupRecordBuffer
getRecordBuffer(HoodieReaderContext readerContext,
-
HoodieTableMetaClient hoodieTableMetaClient,
- RecordMergeMode
recordMergeMode,
- TypedProperties
props,
-
Option<HoodieBaseFile> baseFileOption,
- boolean
hasNoLogFiles,
- boolean
isSkipMerge,
- boolean
shouldUseRecordPosition,
- HoodieReadStats
readStats) {
+ private static FileGroupRecordBuffer getRecordBuffer(HoodieReaderContext
readerContext,
+ HoodieTableMetaClient
hoodieTableMetaClient,
+ RecordMergeMode
recordMergeMode,
+ TypedProperties props,
+ Option<HoodieBaseFile>
baseFileOption,
+ boolean hasNoLogFiles,
+ boolean isSkipMerge,
+ boolean
shouldUseRecordPosition,
+ HoodieReadStats
readStats) {
if (hasNoLogFiles) {
return null;
} else if (isSkipMerge) {
- return new HoodieUnmergedFileGroupRecordBuffer<>(
+ return new UnmergedHoodieFileGroupRecordBuffer<>(
Review Comment:
```suggestion
return new UnmergedFileGroupRecordBuffer<>(
```
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/UnmergedHoodieFileGroupRecordBuffer.java:
##########
@@ -41,12 +41,12 @@
import java.util.Iterator;
import java.util.Map;
-public class HoodieUnmergedFileGroupRecordBuffer<T> extends
HoodieBaseFileGroupRecordBuffer<T> {
+public class UnmergedHoodieFileGroupRecordBuffer<T> extends
FileGroupRecordBuffer<T> {
Review Comment:
```suggestion
public class UnmergedFileGroupRecordBuffer<T> extends
FileGroupRecordBuffer<T> {
```
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/FileGroupRecordBuffer.java:
##########
@@ -143,6 +148,29 @@ public
HoodieBaseFileGroupRecordBuffer(HoodieReaderContext<T> readerContext,
} catch (IOException e) {
throw new HoodieIOException("IOException when creating
ExternalSpillableMap at " + spillableMapBasePath, e);
}
+ this.shouldCheckCustomDeleteMarker = hasCustomDeleteConfigs(props,
readerSchema);
+ if (shouldCheckCustomDeleteMarker) {
+ this.customDeleteMarkerKey = props.getProperty(DELETE_KEY);
+ this.customDeleteMarkerValue = props.getProperty(DELETE_MARKER);
+ }
+ }
+
+ protected boolean hasCustomDeleteConfigs(TypedProperties props, Schema
schema) {
+ // DELETE_KEY and DELETE_MARKER both should be set.
+ if (StringUtils.isNullOrEmpty(props.getProperty(DELETE_KEY))
Review Comment:
nit: fetch `props.getProperty(DELETE_KEY)` once instead of calling
`props.getProperty(DELETE_KEY)` twice in this method
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/FileGroupRecordBuffer.java:
##########
@@ -143,6 +148,29 @@ public
HoodieBaseFileGroupRecordBuffer(HoodieReaderContext<T> readerContext,
} catch (IOException e) {
throw new HoodieIOException("IOException when creating
ExternalSpillableMap at " + spillableMapBasePath, e);
}
+ this.shouldCheckCustomDeleteMarker = hasCustomDeleteConfigs(props,
readerSchema);
+ if (shouldCheckCustomDeleteMarker) {
+ this.customDeleteMarkerKey = props.getProperty(DELETE_KEY);
+ this.customDeleteMarkerValue = props.getProperty(DELETE_MARKER);
+ }
Review Comment:
Could we have these assigning delete marker member variables in a single
method?
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/FileGroupRecordBuffer.java:
##########
@@ -143,6 +148,29 @@ public
HoodieBaseFileGroupRecordBuffer(HoodieReaderContext<T> readerContext,
} catch (IOException e) {
throw new HoodieIOException("IOException when creating
ExternalSpillableMap at " + spillableMapBasePath, e);
}
+ this.shouldCheckCustomDeleteMarker = hasCustomDeleteConfigs(props,
readerSchema);
+ if (shouldCheckCustomDeleteMarker) {
+ this.customDeleteMarkerKey = props.getProperty(DELETE_KEY);
+ this.customDeleteMarkerValue = props.getProperty(DELETE_MARKER);
+ }
+ }
+
+ protected boolean hasCustomDeleteConfigs(TypedProperties props, Schema
schema) {
+ // DELETE_KEY and DELETE_MARKER both should be set.
+ if (StringUtils.isNullOrEmpty(props.getProperty(DELETE_KEY))
+ || StringUtils.isNullOrEmpty(props.getProperty(DELETE_MARKER))) {
+ return false;
Review Comment:
Log a warning message if `DELETE_KEY` is set but `DELETE_MARKER` is not set.
--
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]
