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

add InCond #14

Merged
merged 3 commits into from
Sep 24, 2021
Merged

add InCond #14

merged 3 commits into from
Sep 24, 2021

Conversation

HidetakaKojo
Copy link
Contributor

What this PR does ?

Add a In Condition to Where clauses.

@genkami
Copy link
Owner

genkami commented Sep 21, 2021

Looks nice! Thank you for the PR!

I have one question. What will it be like if we want to use subqueries like WHERE a IN (SELECT b FROM ...)?

I think it would be better to unnest explicitly like:

In(Ident("hoge"), Unnest([]string{"foo", "bar"}))

and define an interface that is responsible for converting memeduck values to ast.InCondition like:

type InCondition interface {
    ToASTInCondition() (ast.InCondition, error)
}

func Unnest(...) *UnnestInCondition { ... }

func (*UnnestInCondition) ToASTInCondition() (ast.InCondition, error) { ... }

for further extensibility.

@HidetakaKojo
Copy link
Contributor Author

HidetakaKojo commented Sep 22, 2021

Thanks for your review.
As you pointed out , I didn't consider the other InCondition type.
I fixed it. :)

Copy link
Owner

@genkami genkami left a comment

Choose a reason for hiding this comment

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

Looks good!
Thank you 🙏

@genkami genkami merged commit 29a5f48 into genkami:main Sep 24, 2021
@HidetakaKojo HidetakaKojo deleted the use-in-cond branch September 24, 2021 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants