Row UDF scalar arguments #311

brandon-b-miller · 2021-11-11T20:58:36Z

Closes #279

brandon-b-miller · 2021-11-15T20:46:17Z

This is ready for review - I thought some code might need to be written to disambiguate when one has a column with a name that could also be a constant, like having a column with name 42 and also passing 42 as a constant parameter. But it turns out we can just write \"42\" in the query to get the column with that name instead.

ayushdg

Thanks a lot for the changes @brandon-b-miller !
Couple of minor comments in addition to the changes suggested:
Would it be possible to also update the docstring to include an example with scalar values?

Also I was able to catch the issues below while testing this with udfs that accept multiple column args, since in the single arg case the output would be correct. Might be worth adding that case to the tests as well.

ayushdg · 2021-11-16T14:02:19Z

dask_sql/datacontainer.py

+                    scalar_args.append(operand)
+                df = column_args[0].to_frame()
+                for col in column_args[1:]:
+                    df[col.name] = operand


Suggested change

df[col.name] = operand

df[col.name] = col

ayushdg · 2021-11-16T14:02:48Z

dask_sql/datacontainer.py

+                df = column_args[0].to_frame()
+                for col in column_args[1:]:
+                    df[col.name] = operand


I'm guessing this section was meant to be outside the loop.

brandon-b-miller · 2021-11-16T14:53:28Z

Updated! :)

charlesbluca

Thanks for the work here @brandon-b-miller 🙂 just small nitpicks, the code itself LGTM

charlesbluca · 2021-11-16T20:37:40Z

tests/integration/test_function.py

+    assert_frame_equal(return_df.reset_index(drop=True), expectation)
+
+
+# Test row UDFs with one args


Suggested change

# Test row UDFs with one args

# Test row UDFs with two args

charlesbluca · 2021-11-16T20:37:47Z

tests/integration/test_function.py

    assert_frame_equal(return_df.reset_index(drop=True), expectation)


+# Test row UDFs with one args


Suggested change

# Test row UDFs with one args

# Test row UDFs with one arg

ayushdg

One small suggestion otherwise lgtm!

ayushdg · 2021-11-16T20:46:35Z

dask_sql/datacontainer.py

+                    column_args.append(operand)
+                else:
+                    scalar_args.append(operand)
+                df = column_args[0].to_frame()


Suggested change

df = column_args[0].to_frame()

df = column_args[0].to_frame()

codecov-commenter · 2021-11-16T21:29:15Z

Codecov Report

Merging #311 (0c5f740) into main (2194a75) will decrease coverage by 0.05%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
- Coverage   95.75%   95.69%   -0.06%     
==========================================
  Files          65       65              
  Lines        2802     2810       +8     
  Branches      418      421       +3     
==========================================
+ Hits         2683     2689       +6     
- Misses         77       78       +1     
- Partials       42       43       +1

Impacted Files	Coverage Δ
dask_sql/datacontainer.py	`94.68% <80.00%> (+0.36%)`	⬆️
dask_sql/server/responses.py	`97.87% <0.00%> (-2.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2194a75...0c5f740. Read the comment docs.

charlesbluca

LGTM; thanks for the work @brandon-b-miller 😄

brandon-b-miller added 2 commits November 11, 2021 11:16

toy impl

ef3e549

tests

c26e5db

ayushdg requested changes Nov 16, 2021

View reviewed changes

brandon-b-miller added 2 commits November 16, 2021 06:42

address reviews

fd10969

add example

92d17f9

charlesbluca reviewed Nov 16, 2021

View reviewed changes

ayushdg approved these changes Nov 16, 2021

View reviewed changes

address reviews

0c5f740

charlesbluca approved these changes Nov 16, 2021

View reviewed changes

charlesbluca merged commit fda0f4f into dask-contrib:main Nov 16, 2021

		assert_frame_equal(return_df.reset_index(drop=True), expectation)


		# Test row UDFs with one args

	df = column_args[0].to_frame()
	df = column_args[0].to_frame()

Row UDF scalar arguments #311

Row UDF scalar arguments #311

Uh oh!

Conversation

brandon-b-miller commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandon-b-miller commented Nov 15, 2021

Uh oh!

ayushdg left a comment

Choose a reason for hiding this comment

Uh oh!

ayushdg Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

ayushdg Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

brandon-b-miller commented Nov 16, 2021

Uh oh!

charlesbluca left a comment

Choose a reason for hiding this comment

Uh oh!

charlesbluca Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

charlesbluca Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

ayushdg left a comment

Choose a reason for hiding this comment

Uh oh!

ayushdg Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

charlesbluca left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brandon-b-miller commented Nov 11, 2021 •

edited

Loading

codecov-commenter commented Nov 16, 2021 •

edited

Loading