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

Drop quotes for UDF metadata #1010

Closed
1 of 2 tasks
xzdandy opened this issue Aug 31, 2023 · 8 comments
Closed
1 of 2 tasks

Drop quotes for UDF metadata #1010

xzdandy opened this issue Aug 31, 2023 · 8 comments
Assignees
Labels
Feature Request ✨ New feature or request
Milestone

Comments

@xzdandy
Copy link
Collaborator

xzdandy commented Aug 31, 2023

Search before asking

  • I have searched the EvaDB issues and found no similar feature requests.

Description

Instead of currently

CREATE UDF IF NOT EXISTS PredictHouseRent FROM
( SELECT * FROM HomeRentals )
TYPE Ludwig
'predict' 'rental_price'
'time_limit' 120;

We'd like to move to

CREATE UDF IF NOT EXISTS PredictHouseRent FROM
( SELECT * FROM HomeRentals )
TYPE Ludwig
PREDICT rental_price
TIME_LIMIT 120;

Use case

No response

Are you willing to submit a PR?

  • Yes I'd like to help by submitting a PR!
@xzdandy xzdandy added the Feature Request ✨ New feature or request label Aug 31, 2023
@xzdandy xzdandy added this to the v0.3.4 milestone Aug 31, 2023
@hershd23
Copy link
Contributor

Hi can I take this up?

@gaurav274
Copy link
Member

Sure!

@hershd23 hershd23 self-assigned this Aug 31, 2023
@hershd23
Copy link
Contributor

hershd23 commented Sep 1, 2023

Hi regarding this issue.

I went through files in the parser as well as the final statement_binder evadb/binder/statement_binder.py . I see that predict right now is being added in UDF metadata

elif child.data == "udf_metadata":

My question is whether I need to move PREDICT to a predicate or let it be in the metadata

@xzdandy
Copy link
Collaborator Author

xzdandy commented Sep 1, 2023

Hi regarding this issue.

I went through files in the parser as well as the final statement_binder evadb/binder/statement_binder.py . I see that predict right now is being added in UDF metadata

elif child.data == "udf_metadata":

My question is whether I need to move PREDICT to a predicate or let it be in the metadata

Hi Hersh, the PREDICT should be in the metadata.

@hershd23
Copy link
Contributor

hershd23 commented Sep 1, 2023

Hi came across this line in the lark file

create_udf: CREATE UDF if_not_exists? udf_name INPUT create_definitions OUTPUT create_definitions TYPE udf_type IMPL udf_impl udf_metadata*

Where udf_metadata is described as

udf_metadata: udf_metadata_key udf_metadata_value

udf_metadata_key: string_literal

udf_metadata_value: string_literal | decimal_literal

I don't think changing the string_literal definition would solve this since it is used everywhere for filepaths etc.

    def string_literal(self, tree):
        text = tree.children[0]
        assert text is not None
        return ConstantValueExpression(text[1:-1], ColumnType.TEXT)

I am thinking I need to create a different expression for this and not define PREDICT and TIME_LIMIT as string literals. Can you give me a few pointers here @xzdandy

@xzdandy
Copy link
Collaborator Author

xzdandy commented Sep 2, 2023

I saw in your pr you have used uid instead of string_literal. Is this still a problem?

@hershd23
Copy link
Contributor

hershd23 commented Sep 2, 2023

No no, I just wanted to link the issue to the PR. That's it. Making it a UID solves the problem

xzdandy added a commit that referenced this issue Sep 5, 2023
Replacing udf_metadata_key from string_literal to ID_LITERAL
`	modified:   evadb/parser/evadb.lark`
Removed .value from key_value_pair[0] post the change in type
`	modified:   evadb/parser/lark_visitor/_functions.py`
Replaced string key to ID_LITERAL in test query
`	modified:   test/unit_tests/parser/test_parser.py`

Solves #1010

---------

Co-authored-by: xzdandy <[email protected]>
@hershd23
Copy link
Contributor

hershd23 commented Sep 5, 2023

Closed as #1026 has been merged into staging

@hershd23 hershd23 closed this as completed Sep 5, 2023
jiashenC pushed a commit that referenced this issue Sep 5, 2023
Replacing udf_metadata_key from string_literal to ID_LITERAL
`	modified:   evadb/parser/evadb.lark`
Removed .value from key_value_pair[0] post the change in type
`	modified:   evadb/parser/lark_visitor/_functions.py`
Replaced string key to ID_LITERAL in test query
`	modified:   test/unit_tests/parser/test_parser.py`

Solves #1010

---------

Co-authored-by: xzdandy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request ✨ New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants