Bug fix + changes in `extractKeyValuePairs` parsing logic by arthurpassos · Pull Request #80657 · ClickHouse/ClickHouse

arthurpassos · 2025-05-21T22:30:23Z

Changelog category (leave one):

Backward Incompatible Change

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Changes in this PR:

Introduce a new argument unexpected_quoting_character_strategy to the extractKeyValuePairs function that controls what happens when a quoting_character is unexpectedly found when reading a non quoted key or value. The value can be one of: invalid, accept or promote. Invalid will discard the key and go back to waiting key state. Accept will treat it as part of the key. Promote will discard previous character and start parsing as a quoted key. I believe the default behavior (and maybe the only behavior) should be INVALID, but in 2023 on Add transition from reading key to reading quoted key when double quotes are found #56423 it was implemented the PROMOTE logic. So, to make it kind of backwards compatible, I created this setting.
After parsing a quoted value, only parse the next key if a pair delimiter is found

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

…ds into state handler

clickhouse-gh · 2025-05-29T16:55:40Z

Workflow [PR], commit [6b8b505]

arthurpassos · 2025-06-10T20:24:24Z

@GrigoryPervakov Hi. Do you happen to have time to review this PR?

ilejn · 2025-06-13T13:09:58Z

src/Functions/keyvaluepair/impl/Configuration.cpp

+{
+    const auto unexpected_quoting_character_strategy_lower = Poco::toLower(unexpected_quoting_character_strategy);
+
+    if (unexpected_quoting_character_strategy_lower == "accept")


Consider using magic_enum - its fun.
In CH it is 'covered' by EnumReflection.h
magic_enum::enum_castConfiguration::UnexpectedQuotingCharacterStrategy(unexpected_quoting_character_strategy, magic_enum::case_insensitive);

Oh, I didn't know it was included in ClickHouse. I remember using magic_enum heavily in ~2018/2019. That's cool.

arthurpassos · 2025-06-13T13:13:48Z

src/Functions/extractKeyValuePairs.cpp


+        if (parsed_arguments.unexpected_quoting_character_strategy)
+        {
+            builder.withUnexpectedQuotingCharacterStrategy(parsed_arguments.unexpected_quoting_character_strategy->getDataAt(0).toString());


I need to assert this is not empty

Actually, this is not a problem. This code is only executed in case the argument has been specified, and empty string will work just fine.

ianton-ru · 2025-06-13T13:43:09Z

docs/en/sql-reference/functions/tuple-map-functions.md

 └──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
 ```

+unexpected_quoting_character_strategy examples:


I think good to add examples with string like
"abc:'5',def":'7'
Can we get single key-value pair with key abc:'5',def?

It depends on what's your quoting character. Is it " or ' or something else? The default is ".

If you are relying on the defaults, it does not really matter what's the value of unexpected_quoting_character_strategy because anything inside a quoted key is valid.

You'll end up with:

Input: "abc:'5',def":'7'
Output: {'abc:'5',def', '7'}

ilejn · 2025-06-13T13:59:57Z

docs/en/sql-reference/functions/tuple-map-functions.md

 - `key_value_delimiter` - Single character delimiting keys and values. Defaults to `:`. [String](../data-types/string.md) or [FixedString](../data-types/fixedstring.md).
 - `pair_delimiters` - Set of character delimiting pairs. Defaults to ` `, `,` and `;`. [String](../data-types/string.md) or [FixedString](../data-types/fixedstring.md).
 - `quoting_character` - Single character used as quoting character. Defaults to `"`. [String](../data-types/string.md) or [FixedString](../data-types/fixedstring.md).
+- `unexpected_quoting_character_strategy` - Strategy to handle quoting characters in unexpected places during `read_key` and `read_value` phase. Possible values: "invalid", "accept" and "promote". Invalid will discard key/value and transition back to `WAITING_KEY` state. Accept will treat it as a normal character. Promote will transition to `READ_QUOTED_{KEY/VALUE}` state and start from next character.


Shouldn't we more explicitly say what it is by default?

Not sure, but I guess that values should be in backticks, like `Invalid` will discard ...

ilejn · 2025-06-13T14:05:26Z

src/Functions/keyvaluepair/ArgumentExtractor.cpp

+            data_column,
+            key_value_delimiter,
+            pair_delimiters,
+            quoting_character,


Not sure about trailing comma

GrigoryPervakov

LGTM, except I'm not sure we need to introduce backward incompatible change if it could be avoided keeping feature without changes

GrigoryPervakov · 2025-06-13T20:28:35Z

src/Functions/keyvaluepair/impl/StateHandlerImpl.h

            else if (isQuotingCharacter(*p))
            {
-                return {next_pos, State::READING_QUOTED_KEY};
+                if (configuration.unexpected_quoting_character_strategy == Configuration::UnexpectedQuotingCharacterStrategy::INVALID)


nit: Maybe switch(configuration.unexpected_quoting_character_strategy) would look better

GrigoryPervakov · 2025-06-13T20:38:08Z

src/Functions/keyvaluepair/impl/KeyValuePairExtractorBuilder.h

    char quoting_character = '"';
    std::vector<char> item_delimiters = {' ', ',', ';'};
    uint64_t max_number_of_pairs = std::numeric_limits<uint64_t>::max();
+    extractKV::Configuration::UnexpectedQuotingCharacterStrategy unexpected_quoting_character_strategy = extractKV::Configuration::UnexpectedQuotingCharacterStrategy::INVALID;


Am I right that this INVALID default is the only reason for backward incompatibility of the PR?
And if we keep it PROMOTE it could be improvement?

Yes, you are correct. It's a choice we gotta make. IMO, INVALID is what should be the default, but unfortunately it is not the behavior that was implemented previously

I see. But it worked well enough, and after this patch, some queries could be broken. Maybe we introduce a new setting like extract_key_value_pairs_default_unexpected_quoting_character_strategy and keep the old behaviour in the changes history.

I don't think we need to break it now. Let's keep the old behaviour by default or make it safe to upgrade.

Ok, I made PROMOTE the default behavior

clickhouse-gh · 2025-06-16T20:16:56Z

Workflow [PR], commit [afafde8]

Summary: ❌

job_name	test_name	status
Stateless tests (amd_asan, distributed plan, 1/2)		failure
	02922_server_exit_code	FAIL
Stateless tests (amd_binary, ParallelReplicas, s3 storage)		failure
	03403_read_in_order_streams_memory_usage	FAIL
Stateless tests (amd_coverage,1/6)		dropped
Stateless tests (amd_coverage,2/6)		dropped
Stateless tests (amd_coverage,3/6)		dropped
Stateless tests (amd_coverage,4/6)		dropped
Stateless tests (amd_coverage,5/6)		dropped
Stateless tests (amd_coverage,6/6)		dropped

arthurpassos · 2025-06-17T18:29:58Z

03403_read_in_order_streams_memory_usage - Not related and also failed on #80840

02922_server_exit_code - Not related and also failed on #74193

@GrigoryPervakov Can we merge it?

…plus_changes_in_extract_kvp_logic 24.8 Partial Backport of ClickHouse#80657 - Wait for pair delimiter after flusing quoted value on extractKeyValuePairs

arthurpassos added 4 commits May 21, 2025 17:18

kind of revert 56423

79c7d30

check for pair delimiter after quoted value and move flush-like metho…

756ace8

…ds into state handler

introduce argument to control behavior

ac56ba1

...

dec9c61

arthurpassos marked this pull request as ready for review May 28, 2025 11:30

arthurpassos changed the title ~~Changes in extractKeyValuePairs parsing logic~~ Bug fix + changes in extractKeyValuePairs parsing logic May 28, 2025

clickhouse-gh bot added the pr-backward-incompatible Pull request with backwards incompatible changes label May 29, 2025

GrigoryPervakov added the can be tested Allows running workflows for external contributors label May 30, 2025

arthurpassos added 2 commits May 30, 2025 16:14

add logical_error extern

1484767

fix test

9bfd94d

ilejn reviewed Jun 13, 2025

View reviewed changes

arthurpassos commented Jun 13, 2025

View reviewed changes

ianton-ru reviewed Jun 13, 2025

View reviewed changes

ilejn reviewed Jun 13, 2025

View reviewed changes

arthurpassos added 3 commits June 13, 2025 11:07

specify default value on ndocs

3e1f223

remove trailing comma

9964b4b

use magic_enum

b80e720

GrigoryPervakov reviewed Jun 13, 2025

View reviewed changes

GrigoryPervakov self-assigned this Jun 13, 2025

switch case instead of if statements

6b8b505

arthurpassos force-pushed the fixes_extract_kvp branch from 875c486 to 6b8b505 Compare June 13, 2025 21:39

keep promote as the default behavior to keep it backwards compatible

b50a0e6

fix ut

afafde8

arthurpassos mentioned this pull request Jun 17, 2025

24.8 Partial Backport of #80657 - Wait for pair delimiter after flusing quoted value on extractKeyValuePairs Altinity/ClickHouse#865

Merged

13 tasks

GrigoryPervakov approved these changes Jun 17, 2025

View reviewed changes

GrigoryPervakov enabled auto-merge June 17, 2025 21:40

GrigoryPervakov added this pull request to the merge queue Jun 17, 2025

Merged via the queue into ClickHouse:master with commit 0aa82b3 Jun 17, 2025
118 of 123 checks passed

robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 17, 2025

zvonand mentioned this pull request Jun 23, 2025

24.8.14 Pre-release PR Altinity/ClickHouse#827

Merged

Conversation

arthurpassos commented May 21, 2025 • edited by fm4v Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Documentation entry for user-facing changes

Uh oh!

clickhouse-gh bot commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurpassos commented Jun 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GrigoryPervakov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh bot commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurpassos commented Jun 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arthurpassos commented May 21, 2025 •

edited by fm4v

Loading

clickhouse-gh bot commented May 29, 2025 •

edited

Loading

clickhouse-gh bot commented Jun 16, 2025 •

edited

Loading