Apply `.WithSettings()` after query syntax

[danthegoodman1]

trafficstars

I noticed that when using

ctx := clickhouse.Context(context.Background(), clickhouse.WithSettings(clickhouse.Settings{
		"additional_table_filters": "{'sec_filings' : 'cik != \\'1449715\\''}",
	})
...
rows, err := conn.Query(ctx, `
	SELECT
    filing_type,
    filing_date,
    accepted_time
FROM sec_filings
WHERE cik = '1449715'
ORDER BY filing_date DESC
LIMIT 100
FORMAT PrettyCompactMonoBlock
SETTINGS additional_table_filters={'sec_filings' : 'cik = \'1449715\''}
	`)

the rows with cik = '1449715' are present. If I invert them (to where the .WithSettings() filters in and the query settings filter out), they are removed from the results.

I feel that the .WithSettings() should be applied after the query SETTINGS, at least overriding matching ones. Maybe this can be configurable with an .WithSettingsOverride() and leaving .WithSettings() with its current behavior.

The use case for this is if you want to have customers writing clickhouse queries (that are already well limited with row policies and user permissions), but want to apply extra filters on top at query time, like a multi-tenant table.

[gingerwizard]

@mshustov this is something for which we should be consistent across clients

It makes more sense to me for local query settings to override connection-level settings - its been the behavior for a long time and this is the first report of an issue.

Saying that I understand @danthegoodman1 use case. I think @danthegoodman1 is proposing a .WithProtectedSettings ? i.e. settings which can't be overridden at a query level.

[mshustov]

It makes more sense to me for local query settings to override connection-level settings - its been the behavior for a long time and this is the first report of an issue.

Agree, a more specific use case takes precedence.

The use case for this is if you want to have customers writing clickhouse queries (that are already well limited with row policies and user permissions),

How critical is it in your case to allow users to change settings at all? Changing some settings can affect the entire server, not just a single query.

Maybe this can be configurable with an .WithSettingsOverride()

How would you expect WithSettingsOverride to behave? Should additional_table_filters content be merged or just overridden?

// merge
additional_table_filters={'a' : '...'}
additional_table_filters={'b' : '...'}
// final
additional_table_filters={'a' : '...', 'b' : '...' }


// override
additional_table_filters={'a' : '...'}
additional_table_filters={'b' : '...'}
// final
additional_table_filters={'b' : '...' }

[danthegoodman1]

@gingerwizard @mshustov thanks so much for the swift responses!!

How critical is it in your case to allow users to change settings at all? Changing some settings can affect the entire server, not just a single query.

Don't need them to change at all tbh, but I wouldn't want to have the query capability limited to just "allow settings in query or no". Right now I would just bail if the string settings exists (without quotes) but that's a very hacky solution and I'm sure there is some fancy query syntax that can get around this.

How would you expect WithSettingsOverride to behave? Should additional_table_filters content be merged or just overridden?

Replace for sure to be safe. The idea here is that I can trust that these will be the exact settings applied.

I think even a third case where it replaces ALL settings in the query. I.e. if settings were in the query, but a WithSettingsOverride existed then the settings in the query string would be completely removed and replaced. Then we could use WithProtectedSettings to serve as a strict replacement without removing untouched settings from the query.

E.g. it might look like this:

.WithProtectedSettings()

// protected settings
set1='a'

// query string settings
set1='b'
set2='c'

// final result

set1='a'
set2='c'

.WithSettingsOverride()

// override settings
set1='a'

// query string settings
set1='b'
set2='c'

// final result
set1='a'

[gingerwizard]

.WithProtectedSettings() makes the most sense to me. .WithSettingsOverride() is confusing - it just means query level settings are ignored or would the intention for this to be applied at the query level?

[danthegoodman1]

I would do this in the query context, so at a per-query level. I don't intend to use these at the connection level

[gingerwizard]

Ok but for your original use case .WithProtectedSettings() would resolve the issue. Those settings pertaining to row security would be applied here (you should really use CH inbuilt row security for this btw) - any subsequent settings at a query level would be append-only (and no override identical keys).

We will discuss internally as we'd want to be consistent across clients @mshustov

[danthegoodman1]

Ok but for your original use case .WithProtectedSettings() would resolve the issue. Those settings pertaining to row security would be applied here (you should really use CH inbuilt row security for this btw) - any subsequent settings at a query level would be append-only (and no override identical keys).

We will discuss internally as we'd want to be consistent across clients @mshustov

Yeah the only case row level security is not used is when filtering by something that has high cardinality (like userid). Definitely open to better suggestions for multi-tenant tables though!

[gingerwizard]

Open to accepting a PR for this as I think it makes sense. Otherwise it will likely not make it till December timeframe.

[jkaflik]

Hi @danthegoodman1

We will have an internal discussion on this one soon. (probably next week) As mentioned by @gingerwizard, we would like to make sure we are consistent with other clients.

[jkaflik]

Note: follow up of https://clickhousedb.slack.com/archives/CU478UEQZ/p1670946830293399?thread_ts=1670472716.175689&cid=CU478UEQZ

---

~there is some confusion on clickhouse.Settings usage since they are applied on a query level. According to https://clickhouse.com/docs/en/operations/settings/:~

Make settings in the SETTINGS clause of the SELECT query. The setting value is applied only to that query and is reset to default or previous value after the query is executed.

~it works only for SELECT queries. We need to find a better way of handling that. (SET clause?) For now, I will submit a doc update with clarification.~

[piyushdatazip]

@jkaflik settings such as date_time_input_format = 'best_effort' are used on insert queries.

[jkaflik]

@piyushdatazip you are correct. My apologies, I confused a SQL statement with the protocol capabilities. I did a round of testing and it works as expected.

[danthegoodman1]

Hey have there been any more discussions internally on this?

[jkaflik]

@danthegoodman1 we will discuss this topic early next week.

[jkaflik]

Hi @danthegoodman1

We discussed this feature request last week. We will not implement any additional API for this since we would like to keep ourselves as most compliant with what ClickHouse does. Query string takes precedence here, and we won't change it.

Walkaround would be if you parse the query and remove/modify the necessary settings.

[danthegoodman1]

Understood. Is there a suggested query parse for Go @jkaflik ?

[jkaflik]

@danthegoodman1 I never did that before. Probably some SQL AST like this: https://pkg.go.dev/github.com/chrislusf/gleam/sql/ast Not sure if it would support ClickHouse SQL statements (SETTINGS etc.)

[danthegoodman1]

Yeah that’s my guess as well…

On Tue, Mar 14, 2023 at 6:49 AM Kuba Kaflik @.***> wrote:

@danthegoodman1 https://github.com/danthegoodman1 I never did that before. Probably some SQL AST like this: https://pkg.go.dev/github.com/chrislusf/gleam/sql/ast Not sure if it would support ClickHouse SQL statements (SETTINGS etc.)

— Reply to this email directly, view it on GitHub https://github.com/ClickHouse/clickhouse-go/issues/773#issuecomment-1467858565, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEH3JLH3VKXN4QK2BNFJOJLW4BEKFANCNFSM6AAAAAAQ3ARTAI . You are receiving this because you were mentioned.Message ID: @.***>