[CALCITE-3323] Handle arbitrary/unknown functions that have ordinary syntax#1522
[CALCITE-3323] Handle arbitrary/unknown functions that have ordinary syntax#1522fib-seq wants to merge 1 commit intoapache:masterfrom
Conversation
|
@julianhyde these are the fixes to the calcite code that for passing in context properly for connConfig() within PlannerImpl. |
|
Since CALCITE-3323 is fixed already, can you please create a new JIRA case. The description is "CalciteConnectionConfigImpl.set obliterates previous property values". The cause is that Can you add a unit test, say in PlannerTest, that if you call set(x, 1) then set(y, 2) on a CalciteConnectionConfigImpl, the result contains x and y. |
| properties1.setProperty((String) connectionProp, (String) connectionValue); | ||
| }); | ||
| if (properties1.getProperty(property.camelName(), null) == null) { | ||
| properties1.setProperty(property.camelName(), value); |
There was a problem hiding this comment.
If there is already the property key, we do not override ?
There was a problem hiding this comment.
I agree, ideally it should override if a key is already present. Part of this hack is due to how connConfig() obliterates the CASE_SENSITIVE and CONFORMANCE defaults that may be passed in through context.
Ideally the flow should be in connConfig():
- set defaults
CASE_SENSITIVEandCONFORMANCE - check context for
CalciteConnectionConfig - if
CalciteConnectionConfigcontext exists, set/override defaults properties within context
vs.
- getting
CalciteConnectionConfigfrom context - set defaults but don't override configs from context
I'm not sure the best approach to get the properties from a CalciteConnectionConfig object. Any suggestions would be appreciated.
| final Properties properties1 = new Properties(); | ||
| this.properties.forEach((connectionProp, connectionValue) -> { | ||
| properties1.setProperty((String) connectionProp, (String) connectionValue); | ||
| }); |
There was a problem hiding this comment.
How about properties.clone() ?
There was a problem hiding this comment.
Yes, clone() looks promising.
There was a problem hiding this comment.
@jinxing64 I've moved this PR here and made the changes you suggested: #1528
4 participants
Added costFactory field and updated set to populate with original properties values