-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Move Parquet related classes to the new package #4073
Conversation
Create a new package: pinot-parquet for all Parquet related classes Remove the dependency of parquet-avro from pinot-common
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.
LGTM otherwise
throws IOException { | ||
Path dataFsPath = new Path(fileName); | ||
return AvroParquetReader.<GenericRecord>builder(dataFsPath).disableCompatibility().withDataModel(GenericData.get()) | ||
//noinspection unchecked |
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.
is this added by IDE?
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.
IDE can read this and suppress the warning for this cast.
recordReaderPath, fileFormat.toString()); | ||
// NOTE: we currently have default file format set to AVRO inside segment generator config, do not want to break | ||
// this behavior for clients. | ||
LOGGER |
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.
Previous format looks better to me :)
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 agree, but just follow the auto-format...
@@ -101,8 +101,7 @@ public void setup(Context context) | |||
if (readerConfigFile != null) { | |||
_readerConfigFile = new Path(readerConfigFile); | |||
} | |||
|
|||
_recordReaderPath = _jobConf.get(JobConfigConstants.RECORD_READER_PATH, null); | |||
_recordReaderPath = _jobConf.get(JobConfigConstants.RECORD_READER_PATH); |
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.
RECORD_READER_CLASS_NAME
seems to be more correct name... But, I guess that we are already using this config?
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, so cannot change it lol
TODO: Let's add the documentation on adding a new record reader and link orc/parquet prs as examples. |
Create a new package: pinot-parquet for all Parquet related classes
Remove the dependency of parquet-avro from pinot-common