[SPARK-19944][SQL] Move SQLConf from sql/core to sql/catalyst #17285
Conversation
|
Test build #74484 has finished for PR 17285 at commit
|
|
Test build #74485 has finished for PR 17285 at commit
|
| * for ticket SPARK-19944 (moves SQLConf from sql/core to sql/catalyst). This class should | ||
| * eventually be removed (test cases should just create SQLConf and set values appropriately). | ||
| */ | ||
| case class SimpleCatalystConf( |
There was a problem hiding this comment.
Does SQLConf contain these defaults?
There was a problem hiding this comment.
I think this is just to keep the code change minimal.
There was a problem hiding this comment.
Yeah, I figured as much.
There was a problem hiding this comment.
Yup the purpose is to minimize the change. Otherwise I will change 50+ files ...
| .internal() | ||
| .stringConf | ||
| .createWithDefault(classOf[ParquetOutputCommitter].getName) | ||
| .createWithDefault("org.apache.parquet.hadoop.ParquetOutputCommitter") |
There was a problem hiding this comment.
It is a bit weird to me that we refer to sql/core configs in catalyst. Shouldn't we just move the dynamic configuration and the catalyst only configurations to CatalystConf and make SQLConf extend this?
|
LGTM, merging to master. We should have a follow-up PR to further clean up the code and address some comments. |
|
A late LGTM |
This patch moves SQLConf from sql/core to sql/catalyst. To minimize the changes, the patch used type alias to still keep CatalystConf (as a type alias) and SimpleCatalystConf (as a concrete class that extends SQLConf). Motivation for the change is that it is pretty weird to have SQLConf only in sql/core and then we have to duplicate config options that impact optimizer/analyzer in sql/catalyst using CatalystConf. N/A Author: Reynold Xin <[email protected]> Closes apache#17285 from rxin/SPARK-19944.
## What changes were proposed in this pull request? This is a follow-up of apache#17285 . ## How was this patch tested? existing tests Author: Wenchen Fan <[email protected]> Closes apache#17521 from cloud-fan/conf.
5 participants
What changes were proposed in this pull request?
This patch moves SQLConf from sql/core to sql/catalyst. To minimize the changes, the patch used type alias to still keep CatalystConf (as a type alias) and SimpleCatalystConf (as a concrete class that extends SQLConf).
Motivation for the change is that it is pretty weird to have SQLConf only in sql/core and then we have to duplicate config options that impact optimizer/analyzer in sql/catalyst using CatalystConf.
How was this patch tested?
N/A