Dr 92 add lookup function to athena and big query#651
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support in the Plywood SQL dialect layer to express “lookup”-style operations for BigQuery and AWS Athena, and introduces Turnilo dimension examples/tests for advanced formulas.
Changes:
- Added
lookupExpression(and related helpers) to BigQuery and Athena dialects. - Added new Turnilo fixtures and a Mocha test suite covering parsing/serialization of advanced dimension formulas (lookup, extract, boolean, chained).
- Minor prop-passing change in the Turnilo table visualization (
SplitRowscolor prop).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plywood/client/src/dialect/bigQueryDialect.ts | Adds lookupExpression, searchExpression, and overrides logExpression/constants. |
| plywood/client/src/dialect/awsAthenaDialect.ts | Adds searchExpression and lookupExpression. |
| client/app/components/TurniloComponent/common/models/dimension/advanced-formulas.mocha.ts | New Mocha tests for parsing/round-tripping advanced dimension formulas. |
| client/app/components/TurniloComponent/common/models/dimension/advanced-formula.fixtures.ts | New fixture examples for lookup/extract/boolean/chained formulas. |
| client/app/components/TurniloComponent/client/visualizations/table/table.tsx | Adjusts how colorText is passed to SplitRows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+249
to
+256
| if (base === "e") { | ||
| return `LN(${operand})`; | ||
| } else if (base === "10") { | ||
| return `LOG10(${operand})`; | ||
| } else if (base === "2") { | ||
| return `LOG2(${operand})`; | ||
| } else { | ||
| return `LOG(${this.escapeLiteral(base)}, ${operand})`; |
There was a problem hiding this comment.
logExpression is comparing base to the string "e", but LogExpression passes the SQL for the base expression (e.g. 2.718281828459045 for Math.E), so this branch will never be taken. Also, the fallback LOG(${this.escapeLiteral(base)}, ${operand}) incorrectly quotes the base (turning it into a STRING literal) even though base is a numeric SQL expression; it should be used as-is (and, for BigQuery, likely via LN(operand)/LN(base) or the correct LOG argument order).
Suggested change
| if (base === "e") { | |
| return `LN(${operand})`; | |
| } else if (base === "10") { | |
| return `LOG10(${operand})`; | |
| } else if (base === "2") { | |
| return `LOG2(${operand})`; | |
| } else { | |
| return `LOG(${this.escapeLiteral(base)}, ${operand})`; | |
| const normalizedBase = base.trim(); | |
| if (normalizedBase === "2.718281828459045") { | |
| return `LN(${operand})`; | |
| } else if (normalizedBase === "10") { | |
| return `LOG10(${operand})`; | |
| } else if (normalizedBase === "2") { | |
| return `LOG2(${operand})`; | |
| } else { | |
| return `LN(${operand}) / LN(${normalizedBase})`; |
…into DR-92-add-lookup-function-to-athena-and-big-query
Contributor
Minimum allowed line rate is |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Description
How is this tested?
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)