Skip to content

[CALCITE-3349] Add create and drop function into SqlKind ddl enumerations#1455

Closed
HuangZhenQiu wants to merge 5 commits intoapache:masterfrom
HuangZhenQiu:CALCITE-3349-add-funtion-enum
Closed

[CALCITE-3349] Add create and drop function into SqlKind ddl enumerations#1455
HuangZhenQiu wants to merge 5 commits intoapache:masterfrom
HuangZhenQiu:CALCITE-3349-add-funtion-enum

Conversation

@HuangZhenQiu
Copy link
Contributor

No description provided.

@HuangZhenQiu
Copy link
Contributor Author

@suez1224 Would you please help and review this PR?

@HuangZhenQiu HuangZhenQiu changed the title add create/drop function into SqlKind DDL enum [CALCITE-3349] add create/drop function into SqlKind DDL enum Sep 16, 2019
@HuangZhenQiu
Copy link
Contributor Author

HuangZhenQiu commented Sep 16, 2019

For the details, please refer to https://issues.apache.org/jira/browse/CALCITE-3349.

Copy link
Contributor

@XuQianJin-Stars XuQianJin-Stars left a comment

Choose a reason for hiding this comment

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

+1, It is better to be able to add some tests.

@HuangZhenQiu
Copy link
Contributor Author

@julianhyde
Would you please help to take a look this small PR?

@danny0405 danny0405 changed the title [CALCITE-3349] add create/drop function into SqlKind DDL enum [CALCITE-3349] Add CREATE and DROP FUNCTION into SqlKind DDL enumerations Sep 17, 2019
Copy link
Contributor

@danny0405 danny0405 left a comment

Choose a reason for hiding this comment

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

I don't know how to test either, but i'm okey on this change.

@danny0405 danny0405 added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 17, 2019
@julianhyde
Copy link
Contributor

Be sure to fix capitalization in the commit message.

@HuangZhenQiu HuangZhenQiu changed the title [CALCITE-3349] Add CREATE and DROP FUNCTION into SqlKind DDL enumerations [CALCITE-3349] Add create and drop function into SqlKind ddl enumerations Sep 18, 2019
@HuangZhenQiu
Copy link
Contributor Author

@julianhyde Thanks for pointing it out. I just updated the PR with a new commit log.

*/
public class SqlDropFunction extends SqlDropObject {
public class SqlDropFunction extends SqlDropObject
implements SqlExecutableStatement {

Choose a reason for hiding this comment

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

isn't SqlDropObject already implement SqlExecutableStatement ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is, but does not include create/drop function, we can remove the implements from SqlExecutableStatement.

@julianhyde
Copy link
Contributor

Would it make sense to have a test for adding and dropping functions? Then we can all agree that this change is "complete".

@danny0405
Copy link
Contributor

@HuangZhenQiu Can you follow Julian's suggestion to add some tests ?

@HuangZhenQiu
Copy link
Contributor Author

@julianhyde @danny0405 Sure. Will do in the next revision.

@HuangZhenQiu
Copy link
Contributor Author

@danny0405 @julianhyde
As creating a function in a schema with an external jar needs quite an effort, I just added basic test case in this PR, and leave the full functionality in the following task https://issues.apache.org/jira/browse/CALCITE-3358

} catch (SQLException e) {
assertThat(e.getMessage(),
containsString("Error while executing SQL \"drop function t\":" +
" At line 1, column 15: Type 'T' not found"));
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also throw exception message as "XXX not support" instead "XXX not found".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the default execution logic in SqlDrop for SqlDropFunction, so it will throw function not found in the root schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yean, i mean we do not even support "drop function" then why throws "xxx function not found" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, my mistake, you actually implement the "Drop function". Then we should add more tests about drop function:

Positive tests

  1. Add a function in the schema and test remove with(without) if exists
  2. Test drop function with specified catalog name and database name, i.e. DROP FUNCTION catalog_name.db_name.func_name

Negative tests

  1. Test drop function what does not exist
  2. Test drop function with case_sensive true but not found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danny0405
Agree. If the create function is implemented, more test cases are needed. Currently, as the create function is not there, I can't add and drop function in test case. Thus, I would put the implementation of create and drop function in another PR, and leave this PR simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do not need the create function to be implemented, just put the function explicitly into the schema is ok, then we can add drop function test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danny0405 I enhanced the test cases for drop function. But the namespace lookup of catalog_name.db_name is not handled for all table, function, and view. It requires a bigger change, so I would like to leave it as another PR. How do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with the latest change, thanks.

@HuangZhenQiu HuangZhenQiu force-pushed the CALCITE-3349-add-funtion-enum branch from 578ec23 to 79228e6 Compare September 25, 2019 05:15
@danny0405 danny0405 added tests-missing and removed LGTM-will-merge-soon Overall PR looks OK. Only minor things left. labels Sep 25, 2019
@HuangZhenQiu HuangZhenQiu force-pushed the CALCITE-3349-add-funtion-enum branch 2 times, most recently from 570ba21 to 3448700 Compare September 26, 2019 22:28
@HuangZhenQiu HuangZhenQiu force-pushed the CALCITE-3349-add-funtion-enum branch from 3448700 to 1ff85b6 Compare September 26, 2019 23:14
@danny0405 danny0405 added LGTM-will-merge-soon Overall PR looks OK. Only minor things left. and removed tests-missing labels Sep 29, 2019
@hsyuan hsyuan closed this in c09f293 Oct 3, 2019
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 17, 2019
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 20, 2019
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 20, 2019
wangxlong pushed a commit to wangxlong/calcite that referenced this pull request Feb 13, 2020
jamesstarr pushed a commit to jamesstarr/calcite that referenced this pull request Aug 28, 2025
Close apache#1455

Change-Id: Id3b8cdcf31879db5a8a7db43b1e60bd8e1b33e4e
jamesstarr pushed a commit to jamesstarr/calcite that referenced this pull request Mar 16, 2026
Close apache#1455

Change-Id: Id3b8cdcf31879db5a8a7db43b1e60bd8e1b33e4e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants