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

Deferred queries #52

Merged
merged 6 commits into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
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
37 changes: 30 additions & 7 deletions packages/backend/src/execution/composeExecutionResults.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { AnyDB, AnyTable, QueryResult } from '@synthql/queries';
import {
AnyDB,
AnyQuery,
AnyTable,
DeferredResult,
QueryResult,
} from '@synthql/queries';
import { applyCardinality } from '../query/applyCardinality';
import { assertHasKey } from '../util/asserts/assertHasKey';
import { setIn } from '../util/tree/setIn';
Expand All @@ -13,9 +19,9 @@ export function composeExecutionResults(
composeExecutionResultsRecursively(node, queryResult);
}

return applyCardinality(
return applyCardinalityAndDeferredResult(
queryResult,
tree.root.inputQuery.cardinality ?? 'many',
tree.root.inputQuery,
) as QueryResult<AnyDB, AnyTable>;
}

Expand All @@ -40,13 +46,30 @@ function composeExecutionResultsRecursively(
return true;
};
const rows = result.filter((row) => predicate(row));
return applyCardinality(rows, inputQuery.cardinality ?? 'many', {
query: inputQuery,
row: rows,
});

return applyCardinalityAndDeferredResult(rows, inputQuery);
});

for (const child of node.children) {
composeExecutionResultsRecursively(child, queryResult);
}
}

function applyDeferredQueryResult<T>(
result: T,
defer: boolean = false,
): DeferredResult<T> | T {
if (!defer) {
return result;
}
return { status: 'done', data: result };
}

function applyCardinalityAndDeferredResult<T>(rows: T[], inputQuery: AnyQuery) {
const withCardinality = applyCardinality(rows, inputQuery.cardinality ?? 'many', {
query: inputQuery,
row: rows,
});

return applyDeferredQueryResult(withCardinality, inputQuery.lazy);
}
Comment on lines +68 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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 key row, but it's being assigned an array (rows). This might cause confusion or errors. Consider updating the key to rows to match the array being passed:

 const withCardinality = applyCardinality(rows, inputQuery.cardinality ?? 'many', {
     query: inputQuery,
-    row: rows,
+    rows: rows,
 });

This change ensures consistency between the key name and the value type, potentially preventing issues in the applyCardinality function.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function applyCardinalityAndDeferredResult<T>(rows: T[], inputQuery: AnyQuery) {
const withCardinality = applyCardinality(rows, inputQuery.cardinality ?? 'many', {
query: inputQuery,
row: rows,
});
return applyDeferredQueryResult(withCardinality, inputQuery.lazy);
}
function applyCardinalityAndDeferredResult<T>(rows: T[], inputQuery: AnyQuery) {
const withCardinality = applyCardinality(rows, inputQuery.cardinality ?? 'many', {
query: inputQuery,
rows: rows,
});
return applyDeferredQueryResult(withCardinality, inputQuery.lazy);
}

5 changes: 4 additions & 1 deletion packages/backend/src/execution/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { composeExecutionResults } from './composeExecutionResults';
import { createExecutionPlan } from './planning/createExecutionPlan';
import { executePlan } from './execution/executePlan';
import { QueryExecutor } from './types';
import { shouldYield } from './execution/shouldYield';

export interface ExecuteProps {
executors: Array<QueryExecutor>;
Expand Down Expand Up @@ -40,6 +41,8 @@ export async function* execute<DB, TQuery extends Query<DB>>(
const plan = createExecutionPlan(query, props);

for await (const resultTree of executePlan(plan, props)) {
yield composeExecutionResults(resultTree);
if (shouldYield(resultTree)) {
yield composeExecutionResults(resultTree);
}
}
}
1 change: 1 addition & 0 deletions packages/backend/src/execution/execution/executePlan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export function executePlan(
filters: createFilters(planNode),
inputQuery: planNode.inputQuery,
result: rows,
planNode,
children: [],
};
},
Expand Down
25 changes: 25 additions & 0 deletions packages/backend/src/execution/execution/shouldYield.ts
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
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refactoring shouldYieldNode for improved readability and efficiency.

The shouldYieldNode function correctly implements the logic for determining if a node should yield. However, consider the following suggestions for improvement:

  1. Use early return for the case when not all children are executed.
  2. Simplify the logic by returning the result of allChildrenExecuted directly.
  3. Use Array.prototype.every() for a more declarative approach when checking children.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,10 @@ export class PgExecutor implements QueryExecutor<PgQueryResult> {

try {
if (this.props.logging) {
console.log(format(sql, { language: 'postgresql' }));
console.log(
`[${query.name}]:`,
format(sql, { language: 'postgresql' }),
);
}

const queryResult = await client.query(sql, params);
Expand Down
1 change: 1 addition & 0 deletions packages/backend/src/execution/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ export interface ExecResultNode {
* The original query that was executed.
*/
inputQuery: AnyQuery;
planNode: ExecutionPlanNode;
children: ExecResultNode[];
}

Expand Down
2 changes: 2 additions & 0 deletions packages/backend/src/query/iterateQuery.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,13 +194,15 @@ describe('iterateQuery', () => {
where: {
film_id: 1,
},
cardinality: 'many',
},
expected: [
{
query: {
from: 'film',
select: {},
where: { film_id: 1 },
cardinality: 'many',
},
insertionPath: [],
},
Expand Down
1 change: 0 additions & 1 deletion packages/backend/src/tests/e2e/nxm.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ describe('n x m', () => {
executors: [
new PgExecutor({
defaultSchema: 'public',
logging: true,
pool,
}),
],
Expand Down
1 change: 0 additions & 1 deletion packages/backend/src/tests/e2e/payments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ describe('e2e', () => {
test.skip(describeQuery(q), async () => {
const pgExecutor = new PgExecutor({
defaultSchema: 'public',
logging: true,
pool,
});

Expand Down
39 changes: 39 additions & 0 deletions packages/backend/src/tests/e2e/select.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,4 +164,43 @@ describe('select', () => {
Array.from(expected).sort(comparator),
);
});

test('defer', async () => {
const limit = 20;
const language = from('language')
.columns('name')
.where({ language_id: col('film.language_id') })
.defer()
.first();

const query = from('film')
.columns('title')
.include({ language })
.limit(limit)
.all();

const languageNonDeferred = from('language')
.columns('name')
.where({ language_id: col('film.language_id') })
.first();

const queryNonDeferred = from('film')
.columns('title')
.include({ language: languageNonDeferred })
.limit(limit)
.all();

const result = await run(query);
const resultNonDeferred = await run(queryNonDeferred);

expect(
result.map(
(r) =>
r.language.status === 'done' && {
...r,
language: r.language.data,
},
),
).toEqual(resultNonDeferred);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ describe('e2e', () => {

const pgExecutor = new PgExecutor({
defaultSchema: 'public',
logging: true,
pool,
});
const execProps = {
Expand Down
1 change: 0 additions & 1 deletion packages/backend/src/tests/e2e/store-with-films.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ describe('e2e', () => {

const pgExecutor = new PgExecutor({
defaultSchema: 'public',
logging: true,
pool,
});
const execProps = {
Expand Down
2 changes: 1 addition & 1 deletion packages/docs/static/reference/assets/navigation.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading