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

feat(functions): add map_insert function #15567

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hanxuanliang
Copy link
Contributor

@hanxuanliang hanxuanliang commented May 18, 2024

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

part of #15295

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@github-actions github-actions bot added the pr-feature this PR introduces a new feature to the codebase label May 18, 2024
@hanxuanliang
Copy link
Contributor Author

However, there is such a description in Snowflake:

If updateFlag is set to TRUE, then the existing input key is updated to the input value

But in Databend, do we support dynamically setting config in the query? Alternatively, we can set the config when querying with the bendsql?

@b41sh b41sh self-requested a review May 18, 2024 05:21
@b41sh
Copy link
Member

b41sh commented May 18, 2024

However, there is such a description in Snowflake:

If updateFlag is set to TRUE, then the existing input key is updated to the input value

But in Databend, do we support dynamically setting config in the query? Alternatively, we can set the config when querying with the bendsql?

We can register a new function with four arguments for map_insert, for example st_geometryfromwkb can support two and three args.


registry.register_3_arg_core::<EmptyMapType, GenericType<0>, GenericType<1>, MapType<GenericType<0>, GenericType<1>>, _, _>(
"map_insert",
|_, _, _, _| FunctionDomain::Full,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can build map domain from key domain and value domain.

src/query/functions/src/scalars/map.rs Show resolved Hide resolved
src/query/functions/src/scalars/map.rs Outdated Show resolved Hide resolved
@hanxuanliang hanxuanliang force-pushed the feat/15295_map_insert branch 2 times, most recently from 085cead to f4ae992 Compare May 19, 2024 07:31
@hanxuanliang
Copy link
Contributor Author

But, I have discovered a problem now:

CREATE TABLE map_insert_test(col_str Map(String, String Null) Not Null, col_int Map(String, Int Null) Null);

SELECT map_insert(col_int, 'a', 100)
FROM map_insert_test;

For the NULL map, it is currently stuck and unable to run. However, I tried to modify the domain calculation, but it had no effect. I have no idea here 😣

@b41sh
Copy link
Member

b41sh commented May 19, 2024

But, I have discovered a problem now:

CREATE TABLE map_insert_test(col_str Map(String, String Null) Not Null, col_int Map(String, Int Null) Null);

SELECT map_insert(col_int, 'a', 100)
FROM map_insert_test;

For the NULL map, it is currently stuck and unable to run. However, I tried to modify the domain calculation, but it had no effect. I have no idea here 😣

Both val_domain and insert_value_domain may be nullable, and we need to deal with various cases, such as

match (val_domain, insert_value_domain) {
    (Domain::Nullable(NullableDomain {..}), Domain::Nullable(NullableDomain {..})) => ...,
    (Domain::Nullable(NullableDomain {..}), _) => ...,
    (_, Domain::Nullable(NullableDomain {..})) => ...,
    (_, _) => ...,
}

insert_value_domain.clone(),
)))
},
|_, key, value, ctx| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use vectorize_with_builder_3_arg here, so that we can support column values in this function

&& !key_type.is_decimal()
&& !key_type.is_date_or_date_time()
{
ctx.set_error(0, format!("map keys can not be {}", key_type));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if has error, we should return at here.

@@ -244,4 +252,144 @@ pub fn register(registry: &mut FunctionRegistry) {
.any(|(k, _)| k == key)
},
);

registry.register_3_arg_core::<EmptyMapType, GenericType<0>, GenericType<1>, MapType<GenericType<0>, GenericType<1>>, _, _>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EmptyMapType also need support 4 arguments with update_flag, use register_4_arg_core register another function.

FunctionDomain::Domain(match source_domain.has_null {
true => Some((
insert_key_domain.clone(),
insert_value_domain.clone(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

insert_key_domain and insert_value_domain need merge with the key and value domain in source_domain.


registry.register_passthrough_nullable_3_arg(
"map_insert",
|_, domain1, key_domain, value_domain| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function can be removed, as we already have register_3_arg_core function

);

// grammar: map_insert(map, insert_key, insert_value, allow_update)
registry.register_passthrough_nullable_4_arg(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use register_4_arg_core here like the previous one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants