-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HUDI-5138][WIP][DO NOT MERGE] Table Spec FileSliceReader implementation #7199
Conversation
9c658a9
to
1e10233
Compare
|
||
public class HoodieTableId { | ||
|
||
private final String databaseName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a food for thought: URI scheme in the table naming is usual 3-level "namespace.db.table", we can fold "namespace.db" to just be a qualifier or encode them as standalone concerns depending on how we're envisioning it to be used
|
||
@Override | ||
public FileSliceReader project(InternalSchema schema) { | ||
// should we convert to avro Schema? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can just convert it to Avro schema and then use the one you've already impl'd
public FileSliceReader project(Schema requiredSchema) { | ||
String partitionFields = jobConf.get(hive_metastoreConstants.META_TABLE_PARTITION_COLUMNS, ""); | ||
List<String> partitioningFields = | ||
partitionFields.length() > 0 ? Arrays.stream(partitionFields.split("/")).collect(Collectors.toList()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would "/" be a partition-path field separator for Presto as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct.
if (split instanceof RealtimeSplit) { | ||
HoodieMergedLogRecordScanner logRecordScanner = getMergedLogRecordScanner((RealtimeSplit) split, jobConf, readerSchema); | ||
while (baseRecordIterator.hasNext()) { | ||
logRecordScanner.processNextRecord(baseRecordIterator.next()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'd be able to load whole file in memory. Instead, we should create an "merging iterator" going over the base-file and applying updates on the fly (similar to how Spark's one impl'd)
|
||
import static org.apache.hudi.hadoop.utils.HoodieRealtimeRecordReaderUtils.getMergedLogRecordScanner; | ||
|
||
public class HoodieRealtimeMergedRecordReader extends HoodieHiveFileSliceReader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do an aggregation instead of extension here? I'd suggest we simply encapsulate FSR as a field inside instead of extension, unless we're planning to extend its functionality
rebase
4ea436e
to
42517b9
Compare
Closing it in favor of #7508 |
Change Logs
Implements
FileSliceReader
for Hive as specified in #7080. This is still WIP. Please do not merge.TODO
HoodieRecordMerger
implementation forArrayWritable
Impact
High
Risk level (write none, low medium or high below)
High
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist