databend icon indicating copy to clipboard operation
databend copied to clipboard

feat(function): Implement map_delete function

Open shamb0 opened this issue 9 months ago โ€ข 3 comments

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

Summary

Implement map_delete function

  • Addresses part of 15295

Tests

  • [x] Unit Test
  • [x] Logic Test
  • [ ] Benchmark Test
  • [ ] No Test - Explain why

Type of change

  • [ ] Bug Fix (non-breaking change which fixes an issue)
  • [x] 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

shamb0 avatar May 11 '24 05:05 shamb0

Hi @b41sh,

  • I've completed the first draft of map_delete. Could you please review the code and provide feedback when you get a chance?

  • Let me know if there are any areas that need improvement or alternative approaches to consider.

Thanks!

shamb0 avatar May 11 '24 05:05 shamb0

Hi @b41sh,

  • I've completed the first draft of map_delete. Could you please review the code and provide feedback when you get a chance?
  • Let me know if there are any areas that need improvement or alternative approaches to consider.

Thanks!

Hi @shamb0 , thanks for continuing to contribute code. Since the map_delete function has a variable number of arguments and may have more than one key argument, it is better to use Factory function instead of Builtin function. You can refer to these Factory functions as examples.

https://github.com/datafuselabs/databend/blob/main/src/query/functions/src/scalars/string_multi_args.rs#L88 https://github.com/datafuselabs/databend/blob/main/src/query/functions/src/scalars/string_multi_args.rs#L247 https://github.com/datafuselabs/databend/blob/main/src/query/functions/src/scalars/geo.rs#L311

There are many more implementations of the factory function, and you can find more examples by searching the code base for registry.register_function_factory

The factory function is not written in the same way as the builtin function, it can support more flexible argument types, but it needs to deal with more problems, there are mainly the following steps:

First of all, you need to check whether the input arguments are supported types, for the map_delete function, the number of arguments is at least two, the type of the first argument must be map, the type of the second and later arguments must be the same as the type of the map key, and can not be nullable. If the arguments do not meet these conditions, return NULL, otherwise, we can get args_type and return_type, used to generate FunctionSignature.

The second part of writing the logic for executing the function is that the input args are of type ValueRef, and each argument could be a Scalar or a Column, which needs to be handled in a different way. You also need a builder to store the result data, there are many examples in this part, you can refer to the implementation of other functions.

b41sh avatar May 11 '24 06:05 b41sh

Hi @b41sh,

  • I've completed the first draft of map_delete. Could you please review the code and provide feedback when you get a chance?
  • Let me know if there are any areas that need improvement or alternative approaches to consider.

Thanks!

Hi @shamb0 , thanks for continuing to contribute code. Since the map_delete function has a variable number of arguments and may have more than one key argument, it is better to use Factory function instead of Builtin function. You can refer to these Factory functions as examples.

https://github.com/datafuselabs/databend/blob/main/src/query/functions/src/scalars/string_multi_args.rs#L88 https://github.com/datafuselabs/databend/blob/main/src/query/functions/src/scalars/string_multi_args.rs#L247 https://github.com/datafuselabs/databend/blob/main/src/query/functions/src/scalars/geo.rs#L311

There are many more implementations of the factory function, and you can find more examples by searching the code base for registry.register_function_factory

The factory function is not written in the same way as the builtin function, it can support more flexible argument types, but it needs to deal with more problems, there are mainly the following steps:

First of all, you need to check whether the input arguments are supported types, for the map_delete function, the number of arguments is at least two, the type of the first argument must be map, the type of the second and later arguments must be the same as the type of the map key, and can not be nullable. If the arguments do not meet these conditions, return NULL, otherwise, we can get args_type and return_type, used to generate FunctionSignature.

The second part of writing the logic for executing the function is that the input args are of type ValueRef, and each argument could be a Scalar or a Column, which needs to be handled in a different way. You also need a builder to store the result data, there are many examples in this part, you can refer to the implementation of other functions.

Thanks @b41sh, for sharing those details! I'll take a good look at the requirements and circle back with any questions or updates.

shamb0 avatar May 11 '24 07:05 shamb0

Hi @shamb0 It's been a long time since last update, have you made any progress recently?

b41sh avatar May 27 '24 10:05 b41sh

Hi @shamb0 It's been a long time since last update, have you made any progress recently?

Hi @b41sh,

Sorry for the delay. I've been tied up with some personal matters. I have some updates that are partially complete and will aim to make a full commit in the next few days.

shamb0 avatar May 27 '24 11:05 shamb0

Hi @b41sh,

I've refactored the implementation to use a factory function instead of a built-in function. The proto implementation is ready for your review. Currently, it only supports the key variant of type DataType::String. Since the DataType collection is extensive, could you please suggest which other variants we should support next?

shamb0 avatar May 29 '24 00:05 shamb0

Hi @b41sh,

I've refactored the implementation to use a factory function instead of a built-in function. The proto implementation is ready for your review. Currently, it only supports the key variant of type DataType::String. Since the DataType collection is extensive, could you please suggest which other variants we should support next?

Hi @shamb0 Thank you for your modification. There are still some minor problems that need to be modified. You can have a look at them when you are free.

b41sh avatar May 29 '24 11:05 b41sh

Hi @b41sh, I've refactored the implementation to use a factory function instead of a built-in function. The proto implementation is ready for your review. Currently, it only supports the key variant of type DataType::String. Since the DataType collection is extensive, could you please suggest which other variants we should support next?

Hi @shamb0 Thank you for your modification. There are still some minor problems that need to be modified. You can have a look at them when you are free.

Hi @b41sh,

Thank you for the review comments. I'll incorporate the suggested changes and have an updated version ready for your review soon.

shamb0 avatar Jun 01 '24 04:06 shamb0

Hi @b41sh,

Code refactored based on the review comments. Appreciate the detailed feedback, it helped in making the necessary changes. Please review and let me know if any further modifications are required.

shamb0 avatar Jun 05 '24 13:06 shamb0

Hi @b41sh,

I've addressed all the comments and made the necessary changes. The PR is now ready for In-take review.

Please let me know if there's anything else that needs attention.

Thanks,

shamb0 avatar Jun 07 '24 02:06 shamb0