-
Notifications
You must be signed in to change notification settings - Fork 0
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
Deferred queries #52
Deferred queries #52
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import { ExecResultNode, ExecResultTree } from '../types'; | ||
|
||
export function shouldYield(tree: ExecResultTree): boolean { | ||
return shouldYieldNode(tree.root); | ||
} | ||
|
||
function shouldYieldNode(node: ExecResultNode): boolean { | ||
const plannedChildren = node.planNode.children; | ||
const executedChildren = node.children; | ||
|
||
const allChildrenExecuted = | ||
plannedChildren.length === executedChildren.length; | ||
|
||
if (allChildrenExecuted) { | ||
return true; | ||
} | ||
|
||
for (const child of executedChildren) { | ||
if (!shouldYieldNode(child)) { | ||
return false; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
Comment on lines
+7
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider refactoring The
Here's a suggested refactoring: function shouldYieldNode(node: ExecResultNode): boolean {
const allChildrenExecuted =
node.planNode.children.length === node.children.length;
if (!allChildrenExecuted) {
return node.children.every(shouldYieldNode);
}
return allChildrenExecuted;
} This refactoring maintains the same logic while improving readability and potentially efficiency by avoiding unnecessary iterations. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a minor suggestion: New function for applying cardinality and deferred results.
The
applyCardinalityAndDeferredResult
function correctly combines the cardinality and deferred result logic. However, there's a minor issue that should be addressed:The options object passed to
applyCardinality
uses the keyrow
, but it's being assigned an array (rows
). This might cause confusion or errors. Consider updating the key torows
to match the array being passed:This change ensures consistency between the key name and the value type, potentially preventing issues in the
applyCardinality
function.📝 Committable suggestion