QueryCtx enhancement (Config + Session property) (V1 deprecated)

[kewang1024]

I had some discussion with @xiaoxmeng about the current velox config system and we're thinking of an enhancement:

Current problem

As of now, prestissimo wraps session properties into QueryCtx and pass it to velox for each query.
For connector configs read from config file
-they're passed in separately
-with duplicate configs.
For system config from config file, they're not passed in at all.

Such design makes the API complicated and confusing. We already saw misusage like passing a non-existing config.

Proposed solution

In the proposed solution, we will handle the translation logic in prestissimo layer and provide a clean API for velox to use.

Merge logic details

In the merge logic, it targets to:

• Config sanity check.
• Provide config override logic when session property and config both exist.
• Understand native prefix. [Native] Should we have a separate velox.properties file for Prestissimo ? prestodb/presto#20406

Let me know what you guys think?
@mbasmanova, @majetideepak, @aditi-pandit, @amitkdutta, @spershin

[majetideepak]

@kewang1024 It definitely makes sense for Velox to see a single config entry for each use.
Do you know how Presto Java merges both connectorConfig(from session) and connectorProperties(from file)?
I also noticed that Velox tests do not have an easy way to set QueryCtx config. We need a way to set those here https://github.com/facebookincubator/velox/blob/main/velox/exec/tests/utils/Cursor.cpp#L124

[kewang1024]

@kewang1024 It definitely makes sense for Velox to see a single config entry for each use. Do you know how Presto Java merges both connectorConfig(from session) and connectorProperties(from file)? I also noticed that Velox tests do not have an easy way to set QueryCtx config. We need a way to set those here https://github.com/facebookincubator/velox/blob/main/velox/exec/tests/utils/Cursor.cpp#L124

So java doesn't not do merge technically, but session property's default value comes from config and would override the config value, so in some sense, it still merges.

Thanks for the code pointer, I will keep this one in mind when I implement the above enhancement.

[tdcmeehan]

The following questions are based on my understanding that the majority of work and design is related to Presto (if so, should a corresponding Presto issue be opened):

If Java doesn't do the merge, somehow we would need to configure the coordinator not to validate these special native_ session properties. Any idea how to go about that?

Additionally, would these session properties be returned in the output of SHOW SESSION?

[kewang1024]

If Java doesn't do the merge, somehow we would need to configure the coordinator not to validate these special native_ session properties. Any idea how to go about that?

Additionally, would these session properties be returned in the output of SHOW SESSION?

Yes, it would be returned in the output.

As of now, we already have several native only session property, it's just not being used by Java.

[mbasmanova]

@kewang1024 I find it difficult to follow this discussion. Would it be possible to illustrate the problem and a proposed solution with some examples?

[mbasmanova]

My understanding is that some configuration properties cannot be modified at a query level and have to be fixed for the lifetime of the process. Do we have a proposal for how to handle these?

[aditi-pandit]

Have bunch of questions also

• Many Velox properties like

async-data-cache-enabled

async-data-cache-ssd-gb
async-data-cache-ssd-checkpoint-gb
async-cache-ssd-path
async-cache-ssd-disable-file-cow

are used to configure global Velox structures and not changed at query time. Its not clear how your proposal handles these.

• There are multiple properties that were common between Java worker and Native worker. e.g. join_spill_memory_threshold. In PR [Native] Trim native prefix for system session property prestodb/presto#21023 these are renamed with native prefix. At the SHOW SESSION level how are we separating these ? It would be confusing for a Prestissimo user to see a similar session property of the java worker. Its better to show only native system session properties to the Prestissimo clients.

• What is the purpose of conversion map in the diagram ? If we are separating native system session properties like in PR 21023, then why is this needed ?

[vermapratyush]

Regarding connector config.
How are the configs merged when 2 connectors of the same type is registered?
The properties from both the connectors will be conflicting with one another.

[kewang1024]

@kewang1024 I find it difficult to follow this discussion. Would it be possible to illustrate the problem and a proposed solution with some examples?

Sure, one example is that we can have both config and session property passed in, but we actually only read one of it and ignore the other:
orc-compression-codec
orc_compression_codec

[kewang1024]

Have bunch of questions also

• Many Velox properties like

async-data-cache-enabled

async-data-cache-ssd-gb
async-data-cache-ssd-checkpoint-gb
async-cache-ssd-path
async-cache-ssd-disable-file-cow

are used to configure global Velox structures and not changed at query time. Its not clear how your proposal handles these.

• There are multiple properties that were common between Java worker and Native worker. e.g. join_spill_memory_threshold. In PR [Native] Trim native prefix for system session property prestodb/presto#21023 these are renamed with native prefix. At the SHOW SESSION level how are we separating these ? It would be confusing for a Prestissimo user to see a similar session property of the java worker. Its better to show only native system session properties to the Prestissimo clients.
• What is the purpose of conversion map in the diagram ? If we are separating native system session properties like in PR 21023, then why is this needed ?

1. My current plan hasn't included changes for static config for now, we can do a brainstorming and refactoring for them as well
2. I agree, if most people found it confusing to have both session property, then we can think about only showing native system session properties for Prestissimo. Thanks for the good suggestion.
3. The purpose of conversion map is not for normal session property and native session property, it aims to provide mapping between config and session property
   eg:
   orc-compression-codec
   orc_compression_codec
   we need to be able to understand they're the same, and the value from session property should override the value from config.

[kewang1024]

Regarding connector config. How are the configs merged when 2 connectors of the same type is registered? The properties from both the connectors will be conflicting with one another.

I believe they would have different catalog names of same catalog type

[majetideepak]

Only those variables that have both global and session scope will be merged.

[kewang1024]

Version 2 of this enhancement: #7659