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 map_nulls_to_nil configuration #17

Merged
merged 3 commits into from
Jun 2, 2021
Merged

Conversation

JustinG721
Copy link

The odbc used by Snowflex driver will, by default, return :null for empty values in a query rather than nil, a value native to Elixir. This pull requests adds translation of :null to :nil along with configuration to disable this feature if desired.

@JustinG721 JustinG721 force-pushed the nulls_to_nil_config branch from 94ddca4 to b617ee8 Compare June 2, 2021 18:24
@JustinG721 JustinG721 requested a review from zbarnes757 June 2, 2021 18:25
lib/snowflex.ex Outdated
@@ -75,6 +75,8 @@ defmodule Snowflex do
end

defp process_results({:selected, headers, rows}) do
null_to_nil? = Application.get_env(:snowflex, :map_nulls_to_nil?, true)

Choose a reason for hiding this comment

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

Should this default to false to preserve current behavior and not introduce a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it should

@zbarnes757
Copy link
Contributor

zbarnes757 commented Jun 2, 2021

I think rather than an application-level setting, this can be an option on Snowflex.Connection. We can then extend the Snowflex.sql_query/3 and Snowflex.param_query/4 to accept a keyword list of options as the last argument. That will allow us to easily implement #8 later.

Also, please be sure to update the changelog with the new feature. 🙂

@DavidAntaramian
Copy link
Contributor

I think rather than an application level setting, this can be a option on Snowflex.Connection. We can then extend the Snowflex.sql_query/3 and Snowflex.param_query/4 to accept a keyword list of options as the last argument. That will allow us to easily implement #8 later.

Should it perhaps be both?

I can foresee library consumers not wanting to set the option for each query, so allowing them to set it as a default would be good.

@zbarnes757
Copy link
Contributor

I can foresee library consumers not wanting to set the option for each query, so allowing them to set it as a default would be good.

I am thinking of an option you can set like:

defmodule MyApp.Snowflake.Warehouse do
  use Snowflex.Connection,
    otp_app: :my_app,
    timeout: :timer.minutes(20),
    map_nulls_to_nil?: true
end

Then if the user wants to change per request, they can override with

Warehouse.execute(query, map_nulls_to_nil?: false, timeout: :timer.seconds(1))

@JustinG721
Copy link
Author

I can foresee library consumers not wanting to set the option for each query, so allowing them to set it as a default would be good.

I am thinking of an option you can set like:

defmodule MyApp.Snowflake.Warehouse do
  use Snowflex.Connection,
    otp_app: :my_app,
    timeout: :timer.minutes(20),
    map_nulls_to_nil?: true
end

Then if the user wants to change per request, they can override with

Warehouse.execute(query, map_nulls_to_nil?: false, timeout: :timer.seconds(1))

Good suggestion @zbarnes757 , I decided to add the configuration to the using block of the connection and replace the timeout variable with a keyword list argument containing the timeout and my map_nulls_to_nil? variable. The default has also been updated to false to maintain current functionality.

Copy link
Contributor

@zbarnes757 zbarnes757 left a comment

Choose a reason for hiding this comment

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

just change the order of the function arguments

lib/snowflex.ex Outdated Show resolved Hide resolved
lib/snowflex/connection.ex Outdated Show resolved Hide resolved
@JustinG721 JustinG721 requested a review from zbarnes757 June 2, 2021 20:48
@zbarnes757 zbarnes757 merged commit 72a1d5b into master Jun 2, 2021
@zbarnes757 zbarnes757 deleted the nulls_to_nil_config branch June 2, 2021 20:56
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.

5 participants