-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: estimate the cost of running a tag from web view #87
Conversation
Great work as always, @ashish10alex! 👏 I'm encountering an issue with several models: Here’s an example of the error: Adding the ability to switch currencies would be a great improvement.
For example, I generally keep the SEK <> EUR conversion in mind, but I’d have to look up the rate for GBP. |
I think the issue is caused by this: it fails when a const preOpsQuery = curModel.preOps ? curModel.preOps.join("\n") + ";" : "";
const incrementalPreOpsQuery = curModel.incrementalPreOps
? curModel.incrementalPreOps.join("\n") + ";"
: ""; |
thanks @HampB! , I have added the fix. could you please do one more test to ensure it works now ? |
@HampB , I have added support for multiple currencies as well. I was going to do that in one of the next versions. But your suggestion has persuaded me to expedite it 🙂. Appreciate it if you could test It out |
Nicely done, @ashish10alex! 🎉 There’s still an issue with semicolons ( For example, the following query still breaks: pre_operations {
DECLARE date_checkpoint DATE DEFAULT (DATE("${dataform.projectConfig.vars.fullLoadStartDate}"));
${when(incremental(),
`SET date_checkpoint = (DATE_SUB(CURRENT_DATE(), INTERVAL 1 DAY));`
)}
${when(incremental(),
`DELETE FROM ${self()} WHERE PostingDate >= date_checkpoint;`
)}
} If you dont know the obvious solution, i can look into it more later today. :-) |
@HampB , thanks, that was a good catch! should be fixed now |
Looks good to me now! For future development, as you've mentioned in the PR, it would be great to include the possibility of adding dependencies. Additionally, enabling the estimator to run based on a specific model (instead of a tag) along with its dependencies would be a nice enhancement. |
Co-authored-by: kcrowder1 <kyle.crowder@eastfork.com>
This PR:
- Table/View: Pre operation + Create or replaces a table/view statement + main query
- Partitioned or clustered tables: Pre operations + main query
- Incremental: Incremental pre operation query + Create or replaces a table/view statement + main query
- Partitioned or clustered Incremental table: Incremental pre operation query + main query
- Assertion & Operation : Main query
TODO
Potential v2 features