Skip to content

Conversation

@jdye64
Copy link
Collaborator

@jdye64 jdye64 commented Jan 11, 2022

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2022

Codecov Report

Merging #366 (5843594) into main (631eb82) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #366   +/-   ##
=======================================
  Coverage   95.60%   95.60%           
=======================================
  Files          67       67           
  Lines        2932     2933    +1     
  Branches      546      547    +1     
=======================================
+ Hits         2803     2804    +1     
  Misses         79       79           
  Partials       50       50           
Impacted Files Coverage Δ
dask_sql/physical/rel/logical/join.py 98.31% <100.00%> (+0.01%) ⬆️

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 631eb82...5843594. Read the comment docs.

@jdye64 jdye64 changed the title Fix unary conditional join operations [REVIEW] Fix unary conditional join operations Jan 13, 2022
Copy link
Collaborator

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this @jdye64 🙂 couple comments around the test:

Copy link
Collaborator

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Thanks for the work here @jdye64! LGTM

@randerzander
Copy link
Collaborator

I believe this is intended to fix a StackOverflow condition when running q14.

Unfortunately even when I include this fix, I still get a hang and the DAG is never fully generated. I suspect this may be another CBO infinite loop?

…zed left, then back right, over and over again
@randerzander
Copy link
Collaborator

With @jdye64 's latest changes here, the query works again as expected.

This is good to merge from my perspective.

@charlesbluca
Copy link
Collaborator

Failures seem to be related to CodeCov and not actual tests failing - going to merge this in. Thanks again @jdye64!

@charlesbluca charlesbluca merged commit 8a6302c into dask-contrib:main Jan 24, 2022
@jdye64 jdye64 deleted the stackoverflow_optimization branch May 16, 2022 15:23
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.

4 participants