Conversation
There was a problem hiding this comment.
Pull request overview
Adds backend derivation support across query endpoints, including stable derivation IDs, default/override aliases, and frontend-to-API derivation mapping for the remote adapter.
Changes:
- Introduces a backend derivation registry/helpers and updates SQL builders to project derived expressions with stable output names.
- Updates remote adapter query building to include
derivation+aliasand map UI labels to API derivation IDs. - Expands API tests and documentation to cover derivations, validation, and alias behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/remote-query-adapter.test.js | Adds unit coverage for derivation label→id mapping and default aliases in remote queries. |
| src/DataSource/remote/RemoteQueryAdapter.js | Maps derivation labels to stable ids; includes derivation+alias in tuples/cells/picklist query payloads. |
| src/DataSet/TupleSet.js | Uses derived output names (default field__derivation or alias) when building tuple schemas/field lists. |
| server/tests/test_validation_errors.py | Adds validation tests for unknown/incompatible derivations across tuples/cells/members. |
| server/tests/test_query_tuples_api.py | Adds API tests for tuples derivation default alias and alias override. |
| server/tests/test_query_picklist_api.py | Adds API tests for members derivation default alias and alias override. |
| server/tests/test_query_cells_api.py | Adds API tests for cells row derivation default alias and alias override. |
| server/tests/test_query_builder.py | Adds SQL builder assertions verifying derived expressions + aliasing in tuples/picklist. |
| server/tests/test_derivations.py | Adds direct unit tests for derivation registry helpers and error conditions. |
| server/routers/query.py | Threads derivation/alias through axis window fetching and returns derived output names in responses. |
| server/query_builder.py | Centralizes dimension select building, projects derived expressions, and updates tuples/cells/picklist/export SQL generation. |
| server/models.py | Adds alias + derivation normalization to tuple/axis specs; adds derivation/alias fields to picklist models. |
| server/main.py | Maps derivation validation errors to DERIVATION_NOT_SUPPORTED in validation error handler. |
| server/errors.py | Adds DerivationNotSupportedError with consistent API error code/message. |
| server/derivations.py | Introduces backend derivation registry, output naming, quoting, and type-compatibility checks. |
| docs/server/api-reference.md | Documents new derivation + alias request fields and the new DERIVATION_NOT_SUPPORTED error code. |
You can also share your feedback on Copilot code review. Take the survey.
| "month_name": {"template": "CAST(MONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "month_shortname": {"template": "CAST(MONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "week_num": {"template": "CAST(WEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "day_of_year": {"template": "CAST(DAYOFYEAR({expr}) AS USMALLINT)", "kinds": {"date", "timestamp"}}, | ||
| "day_of_month": {"template": "CAST(DAYOFMONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "day_of_week_num": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "iso_day_of_week": {"template": "CAST(ISODOW({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "day_of_week_name": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "day_of_week_shortname": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "local_date": {"template": "{expr}::DATE", "kinds": {"date", "timestamp"}}, | ||
| "iso_date": {"template": "strftime({expr}, '%x')", "kinds": {"date", "timestamp"}}, |
There was a problem hiding this comment.
month_name, month_shortname, day_of_week_name, and day_of_week_shortname currently return numeric values (MONTH/DAYOFWEEK casts) rather than names/short names as their IDs imply. Also, iso_date uses '%x' (locale-dependent) which is not an ISO-8601 stable format; it should produce a consistent ISO date string (e.g., YYYY-MM-DD). Update these templates to produce the intended textual outputs with stable formatting.
| "month_name": {"template": "CAST(MONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "month_shortname": {"template": "CAST(MONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "week_num": {"template": "CAST(WEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_year": {"template": "CAST(DAYOFYEAR({expr}) AS USMALLINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_month": {"template": "CAST(DAYOFMONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_num": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "iso_day_of_week": {"template": "CAST(ISODOW({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_name": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_shortname": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "local_date": {"template": "{expr}::DATE", "kinds": {"date", "timestamp"}}, | |
| "iso_date": {"template": "strftime({expr}, '%x')", "kinds": {"date", "timestamp"}}, | |
| "month_name": {"template": "strftime({expr}, '%B')", "kinds": {"date", "timestamp"}}, | |
| "month_shortname": {"template": "strftime({expr}, '%b')", "kinds": {"date", "timestamp"}}, | |
| "week_num": {"template": "CAST(WEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_year": {"template": "CAST(DAYOFYEAR({expr}) AS USMALLINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_month": {"template": "CAST(DAYOFMONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_num": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "iso_day_of_week": {"template": "CAST(ISODOW({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_name": {"template": "strftime({expr}, '%A')", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_shortname": {"template": "strftime({expr}, '%a')", "kinds": {"date", "timestamp"}}, | |
| "local_date": {"template": "{expr}::DATE", "kinds": {"date", "timestamp"}}, | |
| "iso_date": {"template": "strftime({expr}, '%Y-%m-%d')", "kinds": {"date", "timestamp"}}, |
| "month_name": {"template": "CAST(MONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "month_shortname": {"template": "CAST(MONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "week_num": {"template": "CAST(WEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "day_of_year": {"template": "CAST(DAYOFYEAR({expr}) AS USMALLINT)", "kinds": {"date", "timestamp"}}, | ||
| "day_of_month": {"template": "CAST(DAYOFMONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "day_of_week_num": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "iso_day_of_week": {"template": "CAST(ISODOW({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "day_of_week_name": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "day_of_week_shortname": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | ||
| "local_date": {"template": "{expr}::DATE", "kinds": {"date", "timestamp"}}, | ||
| "iso_date": {"template": "strftime({expr}, '%x')", "kinds": {"date", "timestamp"}}, |
There was a problem hiding this comment.
month_name, month_shortname, day_of_week_name, and day_of_week_shortname currently return numeric values (MONTH/DAYOFWEEK casts) rather than names/short names as their IDs imply. Also, iso_date uses '%x' (locale-dependent) which is not an ISO-8601 stable format; it should produce a consistent ISO date string (e.g., YYYY-MM-DD). Update these templates to produce the intended textual outputs with stable formatting.
| "month_name": {"template": "CAST(MONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "month_shortname": {"template": "CAST(MONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "week_num": {"template": "CAST(WEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_year": {"template": "CAST(DAYOFYEAR({expr}) AS USMALLINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_month": {"template": "CAST(DAYOFMONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_num": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "iso_day_of_week": {"template": "CAST(ISODOW({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_name": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_shortname": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "local_date": {"template": "{expr}::DATE", "kinds": {"date", "timestamp"}}, | |
| "iso_date": {"template": "strftime({expr}, '%x')", "kinds": {"date", "timestamp"}}, | |
| "month_name": {"template": "strftime({expr}, '%B')", "kinds": {"date", "timestamp"}}, | |
| "month_shortname": {"template": "strftime({expr}, '%b')", "kinds": {"date", "timestamp"}}, | |
| "week_num": {"template": "CAST(WEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_year": {"template": "CAST(DAYOFYEAR({expr}) AS USMALLINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_month": {"template": "CAST(DAYOFMONTH({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_num": {"template": "CAST(DAYOFWEEK({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "iso_day_of_week": {"template": "CAST(ISODOW({expr}) AS UTINYINT)", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_name": {"template": "strftime({expr}, '%A')", "kinds": {"date", "timestamp"}}, | |
| "day_of_week_shortname": {"template": "strftime({expr}, '%a')", "kinds": {"date", "timestamp"}}, | |
| "local_date": {"template": "{expr}::DATE", "kinds": {"date", "timestamp"}}, | |
| "iso_date": {"template": "strftime({expr}, '%Y-%m-%d')", "kinds": {"date", "timestamp"}}, |
| if not dim_cols and not agg_exprs: | ||
| required_columns = set(schema_fields) | ||
| else: | ||
| required_columns = {spec["field"] for spec in row_specs + col_specs} | ||
| required_columns.update(m.field for m in measures) | ||
| required_columns.update(m.sort_by for m in measures if getattr(m, "sort_by", None)) |
There was a problem hiding this comment.
The second if not dim_cols and not agg_exprs: block is unreachable because the function already returns for the same condition a few lines earlier. This is likely leftover from a refactor and makes the required-columns logic dead code; remove the unreachable branch (or remove the early return if the intent is to support empty-dimension/empty-measure queries).
| if not dim_cols and not agg_exprs: | |
| required_columns = set(schema_fields) | |
| else: | |
| required_columns = {spec["field"] for spec in row_specs + col_specs} | |
| required_columns.update(m.field for m in measures) | |
| required_columns.update(m.sort_by for m in measures if getattr(m, "sort_by", None)) | |
| required_columns = {spec["field"] for spec in row_specs + col_specs} | |
| required_columns.update(m.field for m in measures) | |
| required_columns.update(m.sort_by for m in measures if getattr(m, "sort_by", None)) |
| class PicklistQueryBody(BaseModel): | ||
| """Body for /api/v1/datasets/{dataset_id}/query/picklist.""" | ||
|
|
||
| field: str | None = None | ||
| derivation: str | None = None | ||
| alias: str | None = None | ||
| search: str | None = "" |
There was a problem hiding this comment.
Tuples/cells specs normalize derivation to lowercase via a validator, but PicklistQueryBody (and QueryPicklistRequest) does not. This makes derivation handling inconsistent across endpoints (e.g., "Year" works for tuples/cells but can fail for members/picklist). Add the same @field_validator("derivation", mode="before") normalization to the picklist models as well (or factor into a shared mixin/base model).
| static #getRemoteAlias(axisItem) { | ||
| if (!axisItem || !axisItem.derivation) { | ||
| return undefined; | ||
| } | ||
| const derivation = RemoteQueryAdapter.#getRemoteDerivation(axisItem, 'query'); |
There was a problem hiding this comment.
#getRemoteAlias hardcodes the derivation error context to 'query', so unsupported derivations in tuples/cells/picklist will raise an error message that points to the wrong context. Consider passing context into #getRemoteAlias (and callers) or letting callers compute derivation once and reuse it for alias creation to keep error messages accurate.
| static #getRemoteAlias(axisItem) { | |
| if (!axisItem || !axisItem.derivation) { | |
| return undefined; | |
| } | |
| const derivation = RemoteQueryAdapter.#getRemoteDerivation(axisItem, 'query'); | |
| static #getRemoteAlias(axisItem, context) { | |
| if (!axisItem || !axisItem.derivation) { | |
| return undefined; | |
| } | |
| const effectiveContext = context || 'query'; | |
| const derivation = RemoteQueryAdapter.#getRemoteDerivation(axisItem, effectiveContext); |
| def _build_dimension_selects( | ||
| dataset_id: str, | ||
| items: list[Any], | ||
| schema_fields: set[str], | ||
| loc_prefix: list[Any], | ||
| ) -> list[dict[str, str]]: |
There was a problem hiding this comment.
The return type is annotated as list[dict[str, str]], but the dict includes values that are not str (e.g., "sort" can be None, and may be an enum depending on the model). Update the annotation to reflect the actual shapes (e.g., dict[str, Any], a TypedDict, or a small dataclass) to prevent misleading typing and future refactor hazards.
| "output_name": output_name, | ||
| "select_sql": f"{expression} AS {quote_output_name(item.field, getattr(item, 'derivation', None), getattr(item, 'alias', None))}", | ||
| "column_sql": quote_output_name(item.field, getattr(item, "derivation", None), getattr(item, "alias", None)), | ||
| "sort": getattr(item, "sort", None), |
There was a problem hiding this comment.
The return type is annotated as list[dict[str, str]], but the dict includes values that are not str (e.g., "sort" can be None, and may be an enum depending on the model). Update the annotation to reflect the actual shapes (e.g., dict[str, Any], a TypedDict, or a small dataclass) to prevent misleading typing and future refactor hazards.
| field_spec = type("PicklistFieldSpec", (), {"field": field, "derivation": query.derivation, "alias": query.alias})() | ||
| select_spec = _build_dimension_selects(dataset_id, [field_spec], schema_fields, ["body"])[0] | ||
| output_col = select_spec["column_sql"] |
There was a problem hiding this comment.
Creating an ad-hoc class via type(...) to satisfy _build_dimension_selects makes the code harder to read and refactor. Prefer a small internal dataclass/NamedTuple/SimpleNamespace (or update _build_dimension_selects to accept a well-defined protocol/dict) so the picklist path is explicit and type-checkable.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive backend support for data derivations, allowing users to apply various transformations to fields within their query requests. It enhances the API by adding Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces backend support for field derivations, a significant feature enhancement. The changes span the API documentation, error handling, data models, and query construction logic, and are accompanied by a comprehensive set of new tests. My review focuses on improving code consistency, correctness, and readability in the new query building logic.
| if not dim_cols and not agg_exprs: | ||
| required_columns = set(schema_fields) | ||
| else: | ||
| required_columns = {spec["field"] for spec in row_specs + col_specs} | ||
| required_columns.update(m.field for m in measures) | ||
| required_columns.update(m.sort_by for m in measures if getattr(m, "sort_by", None)) |
There was a problem hiding this comment.
The if condition on line 519 is identical to the one on line 516, which causes the function to return. This makes the if block on lines 519-520 unreachable. To fix this and improve clarity, you can remove the conditional logic here and unconditionally execute the logic from the else block, as it's the only path that can be taken at this point.
| if not dim_cols and not agg_exprs: | |
| required_columns = set(schema_fields) | |
| else: | |
| required_columns = {spec["field"] for spec in row_specs + col_specs} | |
| required_columns.update(m.field for m in measures) | |
| required_columns.update(m.sort_by for m in measures if getattr(m, "sort_by", None)) | |
| required_columns = {spec["field"] for spec in row_specs + col_specs} | |
| required_columns.update(m.field for m in measures) | |
| required_columns.update(m.sort_by for m in measures if getattr(m, "sort_by", None)) |
| derivation: str | None = None | ||
| alias: str | None = None |
There was a problem hiding this comment.
For consistency with TupleFieldSpec and AxisField, it would be beneficial to add a field_validator to QueryPicklistRequest to normalize the derivation value to lowercase. This would make the API more robust and predictable.
derivation: str | None = None
alias: str | None = None
@field_validator("derivation", mode="before")
@classmethod
def normalize_derivation(cls, value: str | None) -> str | None:
if isinstance(value, str):
return value.lower()
return value| items: list[Any], | ||
| schema_fields: set[str], | ||
| loc_prefix: list[Any], | ||
| ) -> list[dict[str, str]]: |
There was a problem hiding this comment.
The return type hint for _build_dimension_selects is list[dict[str, str]], but the dictionary being created contains values of other types, such as None or SortDirection for the sort key. Please update the type hint to be more accurate, for example list[dict[str, Any]].
| ) -> list[dict[str, str]]: | |
| ) -> list[dict[str, Any]]: |
| return f"SELECT 1 FROM {_quote(dataset_id)} WHERE FALSE", [] | ||
|
|
||
| col = _quote(field) | ||
| field_spec = type("PicklistFieldSpec", (), {"field": field, "derivation": query.derivation, "alias": query.alias})() |
There was a problem hiding this comment.
Using type(...) to dynamically create an object for _build_dimension_selects is a bit of a hack and can be hard to read. Consider using types.SimpleNamespace for better clarity and intent. This also applies to a similar pattern in build_picklist_count_sql. You'll need to add from types import SimpleNamespace at the top of the file.
| field_spec = type("PicklistFieldSpec", (), {"field": field, "derivation": query.derivation, "alias": query.alias})() | |
| field_spec = SimpleNamespace(field=field, derivation=query.derivation, alias=query.alias) |
| return "SELECT 0", [] | ||
|
|
||
| col = _quote(field) | ||
| field_spec = type("PicklistFieldSpec", (), {"field": field, "derivation": query.derivation, "alias": query.alias})() |
There was a problem hiding this comment.
Similar to build_picklist_sql, using type(...) here is a bit obscure. Using types.SimpleNamespace would make the code more readable. You'll need to add from types import SimpleNamespace at the top of the file if it's not already there.
| field_spec = type("PicklistFieldSpec", (), {"field": field, "derivation": query.derivation, "alias": query.alias})() | |
| field_spec = SimpleNamespace(field=field, derivation=query.derivation, alias=query.alias) |
Fixes #414
Summary
field__derivationaliases or caller overridesValidation
./.venv-server/bin/ruff check server/./.venv-server/bin/pytest server/tests -q./.venv-server/bin/pytest tests/test_backend_benchmark_runner.py -qnpm run test:unitnpm run lint:js