chore: add a utils method to getColumnReader with SQLConf #360
Conversation
| DataType type, ColumnDescriptor descriptor, int batchSize, SQLConf conf) { | ||
| boolean useDecimal128 = (Boolean) CometConf.COMET_USE_DECIMAL_128().get(conf); | ||
| boolean useLazyMaterialization = (Boolean) CometConf.COMET_USE_LAZY_MATERIALIZATION().get(conf); | ||
| boolean useLegacyDateTimestamp = (Boolean) CometConf.COMET_USE_DECIMAL_128().get(conf); |
There was a problem hiding this comment.
Just making sure... Is it correct to use COMET_USE_DECIMAL_128 for useLegacyDateTimestamp?
There was a problem hiding this comment.
I meant to have COMET_EXCEPTION_ON_LEGACY_DATE_TIMESTAMP
| boolean useDecimal128, | ||
| boolean useLazyMaterialization) { | ||
| // TODO: support `useLegacyDateTimestamp` for Iceberg | ||
| DataType type, ColumnDescriptor descriptor, int batchSize, SQLConf conf) { |
There was a problem hiding this comment.
I assume we do not need the old signature? It is ok to change now as we have not made a release yet, but just asking because this is a public method.
There was a problem hiding this comment.
I don't think anybody uses the old signature yet.
There was a problem hiding this comment.
This will make it harder to make the column reader Spark agnostic as you're adding one more point of dependency.
There was a problem hiding this comment.
This will make it harder to make the column reader Spark agnostic as you're adding one more point of dependency.
+1 for this point. Maybe we can accept an options parameters in this class, and pass all options from SQLConf instead?
public static ColumnReader getColumnReader(
DataType type, ColumnDescriptor descriptor, Map<String, String> options) {
...
}In the iceberg-spark side, we can simply call conf.getAllConfs.asJava to this method?
| return getColumnReader( | ||
| type, descriptor, batchSize, useDecimal128, useLazyMaterialization, true); | ||
| type, descriptor, batchSize, useDecimal128, useLazyMaterialization, useLegacyDateTimestamp); | ||
| } | ||
|
|
||
| public static ColumnReader getColumnReader( |
There was a problem hiding this comment.
Yea, maybe you can just replace the old getColumnReader.
| boolean useDecimal128, | ||
| boolean useLazyMaterialization) { | ||
| // TODO: support `useLegacyDateTimestamp` for Iceberg | ||
| DataType type, ColumnDescriptor descriptor, int batchSize, SQLConf conf) { |
There was a problem hiding this comment.
This will make it harder to make the column reader Spark agnostic as you're adding one more point of dependency.
We need a way for users to config |
|
Hmm, I think @parthchandra meant that by adding SQLConf, |
* Use 3.4.3
Which issue does this PR close?
Closes #.
Rationale for this change
Add a Utils method
getColumnReaderwhich can passed inSQLConf, so Iceberg can pass inCometConfforColumnReaderWhat changes are included in this PR?
How are these changes tested?