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 a query struct for managing query data #19

Closed
wants to merge 4 commits into from

Conversation

zbarnes757
Copy link
Contributor

As we add more features to Snowflex, we are having a harder time extending what is there. I thought it might be useful to create an Ecto.Schema that can manage the query data. This allows us to simplify some of the user-facing API and concentrate logic in more manageable (and testable) modules.

This is obviously a breaking change so I wanted to put it to the team to see what they thought. All suggestions are welcome and if we decide we don't like this proposal, I'm happy to throw it away 🙂

@zbarnes757 zbarnes757 added the For Discussion 💡 ❔ A proposal that is open for discussion. label Jun 4, 2021
Copy link
Contributor

@superhawk610 superhawk610 left a comment

Choose a reason for hiding this comment

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

Really like this addition 👍

This provides a great jumping-off point for any future additions, as well as a great place to provide query-specific documentation. Since snowflex is published as a standalone library, is it worth considering not depending on ecto for validation? I would imagine some number of users will be running snowflex in an environment without ecto at all, so pulling it all in just for validation may be undesirable.

Either way, looks good to me!

@dustinfarris
Copy link
Member

dustinfarris commented Jun 4, 2021

What would the distance be between an Ecto.Schema representation vs. a full-on Ecto adapter? Our team has started looking at snowflake insert operations recently and it would be pretty great if we had the full Ecto API available.


Update: prior art: https://github.com/joshuataylor/snowflake_elixir_ecto

@zbarnes757
Copy link
Contributor Author

@superhawk610 I considered that too but when the Ecto team split the SQL stuff into its own package, my understanding was one of the reasons was to enable people to use the data validation features without the need to also bring in all the SQL too.

@dustinfarris I'm not sure but I think it likely gets us closer, no? If we can get our library closer to the community standard, it will certainly be easier to integrate that feature set.

lib/snowflex.ex Outdated Show resolved Hide resolved
@jeremyowensboggs
Copy link

What would the distance be between an Ecto.Schema representation vs. a full-on Ecto adapter? Our team has started looking at snowflake insert operations recently and it would be pretty great if we had the full Ecto API available.

Update: prior art: https://github.com/joshuataylor/snowflake_elixir_ecto

Interesting - that library uses what looks like a pure elixir library that provides the DBConnection behaviour to snowflake.

@jeremyowensboggs
Copy link

Right now, we have two members to the struct - the query text and the list of parameters. Two items are manageable as parameters. What additional data are we planning on adding as we do a query against snowflake?

@dustinfarris
Copy link
Member

distance be between an Ecto.Schema representation vs. a full-on Ecto adapter?

@dustinfarris I'm not sure but I think it likely gets us closer, no? If we can get our library closer to the community standard, it will certainly be easier to integrate that feature set.

This PR would certainly be useful for current day-to-day Snowflex. My knee-jerk reaction though is that this would have to be unwound if we decided to pursue an adapter. Will give it some more thought.

@zbarnes757
Copy link
Contributor Author

I went ahead and took the suggestions to remove the ecto schema. I agree that it was a bit of an overkill with the limited keys, @jeremyowensboggs.

@zbarnes757
Copy link
Contributor Author

closing in favor of #32

@zbarnes757 zbarnes757 closed this Jan 11, 2022
@zbarnes757 zbarnes757 deleted the snowflex-query-struct branch January 11, 2022 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Discussion 💡 ❔ A proposal that is open for discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants