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

Avoid mutation of join slice for separate datasets when joins slice capacity is not yet reached #261

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

fhaifler
Copy link

@fhaifler fhaifler commented Apr 7, 2021

I have encountered a bug, when having two separate SelectDatasets with some common part, they can override one anothers joins. Example datasets (build in order):

  • common w/ joins: a, b, c
  • derived1 (from common) w/ join: d
  • derived2 (from common) w/ join: e

Expected behavior would be to see:

  • common joins: a, b, c
  • derived1 joins: a, b, c, d
  • derived2 joins: a, b, c, e

But the result actually is:

  • common joins: a, b, c
  • derived1 joins: a, b, c, e
  • derived2 joins: a, b, c, e

Therefore derived2 dataset overrides the joins of derived1. This can happen for all additional joins for common parts with number of joins < 2^n. This is consequence of append(ret.joins, jc), which reallocates the data only when cap(ret.joins) is reached.

IMHO reslicing joins to a slice with maxed-out cap should fix this issue (practically enforcing copy-on-write) and (hopefully) shouldn't cause issues elsewhere.

I would be glad if this issue could be fixed soon, so we can continue to use this great library without an issue.

Cheers!

@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #261 (606a815) into master (71f3e01) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #261   +/-   ##
=======================================
  Coverage   96.69%   96.69%           
=======================================
  Files          60       60           
  Lines        3302     3302           
=======================================
  Hits         3193     3193           
  Misses         94       94           
  Partials       15       15           
Impacted Files Coverage Δ
exp/select_clauses.go 94.07% <100.00%> (ø)

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 71f3e01...606a815. Read the comment docs.

@george-oakling
Copy link

this fixes our usecase, would you pls merge it @doug-martin ?

@doug-martin doug-martin merged commit 458000e into doug-martin:master Apr 22, 2021
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.

3 participants