Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix an empty JSON map append using _dummy subcolumn #1115

Closed
wants to merge 2 commits into from
Closed

Conversation

jkaflik
Copy link
Contributor

@jkaflik jkaflik commented Oct 11, 2023

if map is empty, we need to create an empty Tuple to make sure subcolumns protocol is happy
_dummy is a ClickHouse internal name for empty Tuple subcolumn
it has the same effect as INSERT INTO single_json_type_table VALUES ('{}');

resolves #1113

@jkaflik jkaflik changed the title fix/1113 Fix an empty JSON map append using _dummy column Oct 11, 2023
@jkaflik jkaflik changed the title Fix an empty JSON map append using _dummy column Fix an empty JSON map append using _dummy subcolumn Oct 11, 2023
@leodido
Copy link
Contributor

leodido commented Oct 11, 2023

I've also implemented a fix @jkaflik.

Well, I've implemented 2. I'm gonna write them here in case you find them useful.

The first fix is to replicate the behavior for the string {} in the map case (so in the appendStructOrMap function).

// Handle empty maps
if jCol.columns == nil && vData.Len() == 0 {
	jCol.encoding = 1
	jCol.columns = append(jCol.columns, &JSONValue{Interface: &String{}})
	return jCol.columns[0].AppendRow(nil) // `{}` input also works
}

The alternative fix that I've developed is (always in the same spot in the appendStructOrMap function).

if jCol.columns == nil && vData.Len() == 0 {
	jCol.upsertValue("_dummy", "Int8")
	jCol.insertEmptyColumn("_dummy")
	return nil
}

In terms of visual results they're totally equivalent to the one you've implemented. Ie, col4=map[_dummy:0].

In case you prefer any of my 2 solutions you just tell me and I'll send it with a PR. Or feel free to pick any of them (co-authorship would be appreciated :)).

@jkaflik
Copy link
Contributor Author

jkaflik commented Oct 11, 2023

@leodido

Thanks. The second one is cleaner but has a slight overhead on type resolve. In that case, I would not bother about it since the entire JSON implementation is costly anyway.

Feel free to submit your second proposal, but please add a few lines of comments so anyone who reviews it later is not frustrated around this hack. :)
Also, please copy and paste the test case.
If you submit it today, I am ok to merge yours 🚀

@leodido
Copy link
Contributor

leodido commented Oct 11, 2023

On it!

@leodido
Copy link
Contributor

leodido commented Oct 11, 2023

Here it is: #1116

@jkaflik
Copy link
Contributor Author

jkaflik commented Oct 11, 2023

Closed in favour of #1116

@jkaflik jkaflik closed this Oct 11, 2023
@jkaflik jkaflik deleted the fix/1113 branch October 11, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mismatched len of columns when inserting empty map into JSON column
2 participants