feat: alternate PRQL-native implementation of append by:name#6046
Open
kgutwin wants to merge 4 commits into
Open
feat: alternate PRQL-native implementation of append by:name#6046kgutwin wants to merge 4 commits into
append by:name#6046kgutwin wants to merge 4 commits into
Conversation
prql-bot
reviewed
Jun 26, 2026
prql-bot
left a comment
Collaborator
There was a problem hiding this comment.
Nice work — the _append_by_name formulation in std.prql is a clever, fully dialect-independent approach, and the column-alignment via tuple_uniq reads cleanly.
CI is red because Clippy runs with -D warnings and flags two issues, both in this diff (see inline suggestions):
expr: &Box<pl::Expr>triggersclippy::borrowed_box—&pl::Exprworks via deref coercion at the call site.- The trailing
return Err(...)inresolve_ident_wildcardtriggersclippy::needless_return.
A couple of non-blocking notes for whenever you pick this back up (the PR description already flags docs/lineage as pending):
tuple_reverseis added tostd.prqland the resolver but, per the description, isn't used by anything. Adding it permanently expands the language surface speculatively — worth dropping it unless there's a concrete consumer, since it'd otherwise need docs/tests of its own.- Several
log::trace!/log::debug!calls and the newModule::all_nameshelper ("Useful for debugging") look like instrumentation left over from development (e.g. the per-ident... candidatesdump inexpr.rs, thefind_input_by_nametrace inmodule.rs, the tuple-intersection trace intypes.rs). They're trace-gated so harmless at runtime, but worth pruning the ones that were just for bring-up before merge.
| pub fn construct_wildcard_from_lineage( | ||
| &mut self, | ||
| prefix: &[&String], | ||
| expr: &Box<pl::Expr>, |
Collaborator
There was a problem hiding this comment.
Suggested change
| expr: &Box<pl::Expr>, | |
| expr: &pl::Expr, |
clippy::borrowed_box fires here (CI -D warnings). &pl::Expr is sufficient — the call site passes &Box<Expr>, which deref-coerces.
| _ => return Err(ambiguous_error(decls, Some(&ident.name))), | ||
| } | ||
|
|
||
| return Err(Error::new_simple(format!("Unknown relation {ident}"))); |
Collaborator
There was a problem hiding this comment.
Suggested change
| return Err(Error::new_simple(format!("Unknown relation {ident}"))); | |
| Err(Error::new_simple(format!("Unknown relation {ident}"))) |
clippy::needless_return fires on this trailing return (CI -D warnings).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See #5165 for background discussion.
This PR adds a PRQL-native implementation of
append by:namewhich is dialect-independent. This is a counterpart (but not exclusive to) PR #6037 which would add dialect-specific support forUNION ALL BY NAME.This PR has three main categories of changes:
std._append_by_namefunction was added, and the internal definition ofappendwas extended to parse theby:named argument. Whenby:nameis provided, it generates aFuncCallexpression to invokestd._append_by_name.semantic/resolver/expr.rsfor wildcard expansion of relations via their lineage information. This seemed to be necessary in order to implement_param.bottom.*within thestd._append_by_namefunction.tuple_uniqandtuple_reverse. I ended up not needing to usetuple_reversebut I left it in anyway in hopes that someone will find it useful.tuple_uniqtakes a tuple which may have duplicate entries (by alias or by Ident name) and returns either the earliest or latest seen instance of a given name, depending on the value of thetake:named argument.Updates to the documentation are still pending, and there's also a potential question related to lineage that I may work on exploring.