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

fix(join): dereference literals in values of JoinChain #9594

Closed
wants to merge 1 commit into from

Conversation

coroa
Copy link
Contributor

@coroa coroa commented Jul 15, 2024

Description of changes

Literals as value of a JoinChain should be substituted through subsequent joins like in the example illustrated in #9561 .

A statement like
t3 = t1.inner_join(t2, "i").select("i", lit=1) creates a JoinChain looking like

JoinChain[r0]
  JoinLink[inner, r1]
    r0.i == r1.i
  values:
    i:     r0.i
    lit:   1

with one of the reference "fields" in values actually pointing to a Literal.

Adding a subsequent JoinLink to this JoinChain like by
t5 = t3.inner_join(t4, "i") fails in prepare_predicates, since the DerefMap.from_targets(..., extra=reverse) call tries to add a substitute from the new lit-field to the Literal(1) object, which does not fit the Field type, that the subs attribute of the DerefMap are expected to have.

Allowing also Literal fixes this specific example.

NOTE: I am wondering if general Values should be allowed.

In general, I am very unsure about the greater implications.

Will think about a small test.

Issues closed

@cpcloud cpcloud added this to the 9.2 milestone Jul 15, 2024
@cpcloud cpcloud added the bug Incorrect behavior inside of ibis label Jul 15, 2024
@cpcloud
Copy link
Member

cpcloud commented Jul 15, 2024

I hopefully-not-too-optimistically labeled this PR for the 9.2 milestone 😬.

@coroa
Copy link
Contributor Author

coroa commented Jul 15, 2024

After some more thinking, I guess the alternative over at #9595 is more correct and I'd prefer to close this one.

But someone more experienced than me will have to make the correct call.

@cpcloud
Copy link
Member

cpcloud commented Jul 15, 2024

@coroa You're right, the other one is more correct because subs is intentionally restricted to Fields alone. (though both actually satisfy my complex test case).

Closing.

@cpcloud cpcloud closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior inside of ibis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: SignatureValidationError from DerefMap during repeated joins
2 participants