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

Use petgraph::algo::tarjan_scc to order the schema graph #1867

Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 33 additions & 9 deletions pgrx-sql-entity-graph/src/pgrx_sql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,19 +403,43 @@ impl PgrxSql {

pub fn to_sql(&self) -> eyre::Result<String> {
let mut full_sql = String::new();
for step_id in petgraph::algo::toposort(&self.graph, None).map_err(|e| {
eyre!("Failed to toposort SQL entities, node with cycle: {:?}", self.graph[e.node_id()])
})? {
let step = &self.graph[step_id];

let sql = step.to_sql(self)?;
full_sql.push_str(sql.trim_matches('\n'));
// NB: A properly we'd *like* to maintain is that the schema generator outputs
// consistent results from run-to-run when there are no changes to the schema.
// This is to improve change detection using simple tools like `diff`.
//
// Historically, we used [`petgraph::algo:toposort`] but its ordering is not at all
// consistent.
//
// [`petgraph::algo::tarjan_scc`] appears to be consistent, although it's not exactly
// clear if this is due to an implementation detail or specifics of the algorithm itself.
// (I, eeeebbbbrrrr, am not a graph theory expert)
//
// In any event, if in the future schema generation stops being consistent, this is the
// place to look.
//
// We have no tests around this as it's really just a property we'd like to have, and
// it does seem ensuring it is a bit of black magic.
for nodes in petgraph::algo::tarjan_scc(&self.graph).iter().rev() {
let mut inner_sql = Vec::with_capacity(nodes.len());
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved

for node in nodes {
let step = &self.graph[*node];
let sql = step.to_sql(self)?;

let trimmed = sql.trim();
if !trimmed.is_empty() {
inner_sql.push(format!("{trimmed}\n"))
}
}

if !full_sql.is_empty() && !full_sql.ends_with("\n\n") {
full_sql.push_str("\n\n");
if !inner_sql.is_empty() {
full_sql.push_str("/* <begin connected objects> */\n");
full_sql.push_str(&inner_sql.join("\n\n"));
full_sql.push_str("/* </end connected objects> */\n\n");
}
}
let _ = full_sql.pop();

Ok(full_sql)
}

Expand Down
Loading