Introduce ASOF and ASOF LEFT joins with single-inequality anchor#27703
Introduce ASOF and ASOF LEFT joins with single-inequality anchor#27703Pluies wants to merge 1 commit into
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
- Adds ASOF/ASOF LEFT joins for nearest‑neighbor matching. - ON clause requires exactly one inequality comparing a right‑side expression to a left‑side expression; additional equi and same‑side predicates are allowed. - Implements analyzer/planner support and predicate pushdown semantics. - Updates documentation and adds focused analyzer, planner, and query tests.
|
That's a cool feature!
Would be nice if we could have one PR to collaborate on. Can you please chime in on #27641 too? |
I believe this PR is slightly more production-ready. My understanding is:
But these could all be ported between PR if needed. I believe the main difference is in the way these PRs approach the issue in the first place: this PR supports ASOF joins as a native construct, whereas #27641 supports ASOF joins by means of rewriting them into a LEFT JOIN via RewriteAsofJoinToLeftJoinWithTop1.java ; an SQL syntax that Trino already understands. cc @jirislav , let me know if I misunderstood changes 🙏
Linking this comment there! |
|
I agree with @Pluies about this PR being more production-ready. My PR (#27641) was intended to only establish the syntax with a simple translation to a LEFT JOIN, so we could ship the improvements of ASOF join implementation incrementally (for example by shipping a sort-merge operator using binary search).
It is only a requirement when the user chooses the Note that having the time-dimension as the last one after the
|
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
|
@Pluies what is the reason to close this? Do we have some alternative PR somewhere I'm not aware of? |
|
@jirislav sorry, my bad - I was cleaning up branches in my fork and deleted this one by mistake 🤦 |
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
kasiafi
left a comment
kasiafi
left a comment
There was a problem hiding this comment.
Hi @Pluies, I have some high level comments/questions:
-
The syntax. I understand that the proposed syntax is something invented and adopted by other DBs. One thing I don't like about this syntax is that the "ASOF-inequality" does not stand out in any way from other join conditions, while it's semantics is very different. Are there alternative syntax approaches that consider this?
-
The "ASOF-inequality" also doesn't get any special treatment in the planner -- it becomes a regular part of the Join filter. And then it has to be extracted again after the query is optimized. This approach seems brittle. What if the inequality gets transformed or optimized-out altogether? What if some optimization adds another inequality to the Join filter? I think that the "ASOF-inequality" must be carried separately from the Join filter if we want to be sure that we still have it on the other end of the Planner.
-
Where is the logic responsible for returning one match for each row?
-
What happens if there are other join conditions than equalities/inequalities?
-
If there are ties on the build side, how are they solved?
-
If there are ties on the probe side matching ties on the build side, is the result consistent for each tied probe row?
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
5 participants
Description
Additional context and related issues
Hey folks 👋 Seeing #27641 being pushed in order to implement #21759, I'm rushing our plans of upstreaming ASOF joins as implemented recently in Dune 🎁
We have developed and built this feature on top of Trino 467, and this is a quick rebase on top of
masterto get the ball rolling. Expect potential bugs and test failures; I'll come back and clean this up asap.I'm not expecting either this PR or #27641 to be adopted wholesale, but hopefully this can help in the conversation wrt which syntax and implementation of ASOF joins ends up in Trino!
cc @martint
Related, our user-facing Dune docs: https://docs.dune.com/query-engine/Functions-and-operators/asof-join
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: