Skip to content

feat: alternate PRQL-native implementation of append by:name#6046

Open
kgutwin wants to merge 4 commits into
PRQL:mainfrom
bioteam:kg/5165/append-by-name-native-prql
Open

feat: alternate PRQL-native implementation of append by:name#6046
kgutwin wants to merge 4 commits into
PRQL:mainfrom
bioteam:kg/5165/append-by-name-native-prql

Conversation

@kgutwin

@kgutwin kgutwin commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

See #5165 for background discussion.

This PR adds a PRQL-native implementation of append by:name which is dialect-independent. This is a counterpart (but not exclusive to) PR #6037 which would add dialect-specific support for UNION ALL BY NAME.

This PR has three main categories of changes:

  1. The std._append_by_name function was added, and the internal definition of append was extended to parse the by: named argument. When by:name is provided, it generates a FuncCall expression to invoke std._append_by_name.
  2. Support was added in semantic/resolver/expr.rs for wildcard expansion of relations via their lineage information. This seemed to be necessary in order to implement _param.bottom.* within the std._append_by_name function.
  3. Two new tuple functions were added: tuple_uniq and tuple_reverse. I ended up not needing to use tuple_reverse but I left it in anyway in hopes that someone will find it useful. tuple_uniq takes 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 the take: 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.

@prql-bot prql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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> triggers clippy::borrowed_box&pl::Expr works via deref coercion at the call site.
  • The trailing return Err(...) in resolve_ident_wildcard triggers clippy::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_reverse is added to std.prql and 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 new Module::all_names helper ("Useful for debugging") look like instrumentation left over from development (e.g. the per-ident ... candidates dump in expr.rs, the find_input_by_name trace in module.rs, the tuple-intersection trace in types.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>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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}")));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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).

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.

2 participants