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(query): Procedure Part2 support arguments #16453

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

TCeason
Copy link
Collaborator

@TCeason TCeason commented Sep 14, 2024

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

Summary

Parat II of #16348

this pr support call procedure();

After this pr, we need to add a new issue to enhanced procedure checks to make it more of a product than a prototype

fixes: Part of #14904

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

@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Sep 14, 2024
@TCeason TCeason marked this pull request as draft September 14, 2024 03:31
@github-actions github-actions bot added the pr-feature this PR introduces a new feature to the codebase label Sep 14, 2024
@dosubot dosubot bot added the A-query Area: databend query label Sep 14, 2024
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 24 files at r1, all commit messages.
Reviewable status: 12 of 24 files reviewed, 2 unresolved discussions (waiting on @TCeason)


src/meta/app/src/principal/procedure.rs line 76 at r1 (raw file):

            "Lanuage: {:?}, args {:?} return_type: {:?}, CreatedOn: {:?}, Script: {:?}, Comment: {:?}",
            self.procedure_language,
            self.arg_names,

use DisplaySliceExt::display_n::<1000>() to build better for human output.

Other fields can also be benefitted from this method.

Suggestion:

            "Lanuage: {:?}, args {} return_type: {:?}, CreatedOn: {:?}, Script: {:?}, Comment: {:?}",
            self.procedure_language,
            self.arg_names.display_n::<1000>(),

src/meta/app/src/principal/procedure_identity.rs line 36 at r1 (raw file):

        Self {
            name: name.to_string(),
            args: args.to_string().to_lowercase(),

Why is args special and is converted to lower cases?

@TCeason
Copy link
Collaborator Author

TCeason commented Sep 14, 2024

Why is args special and is converted to lower cases?

No need to modify it. Already convert to lower in parse create/drop procedure.

e.g.

create procedure p1(x UInt8) -> should can be drop procedure p1(uint8)

@TCeason TCeason force-pushed the procedure2 branch 2 times, most recently from fd6808b to f8b2336 Compare September 14, 2024 10:34
@TCeason
Copy link
Collaborator Author

TCeason commented Sep 14, 2024

warit #16455 merged.

@TCeason TCeason marked this pull request as ready for review September 14, 2024 14:05
@dosubot dosubot bot added the C-feature Category: feature label Sep 14, 2024
@TCeason TCeason force-pushed the procedure2 branch 2 times, most recently from 635e171 to 6463b06 Compare September 15, 2024 00:27
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 24 files at r1, 1 of 3 files at r2, 3 of 4 files at r3, 7 of 9 files at r5, all commit messages.
Reviewable status: 24 of 28 files reviewed, 4 unresolved discussions (waiting on @andylokandy and @sundy-li)

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 18, 2024
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Sep 19, 2024
@sundy-li sundy-li added this pull request to the merge queue Sep 19, 2024
@BohuTANG BohuTANG removed this pull request from the merge queue due to a manual request Sep 19, 2024
@BohuTANG BohuTANG merged commit 56efa54 into databendlabs:main Sep 19, 2024
78 of 79 checks passed
@sundy-li
Copy link
Member

sundy-li commented Sep 19, 2024

docs cc @soyeric128 with @TCeason

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query Area: databend query C-feature Category: feature lgtm This PR has been approved by a maintainer pr-feature this PR introduces a new feature to the codebase size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants