-
Notifications
You must be signed in to change notification settings - Fork 32
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
Materialized connector inital setup #417
Conversation
|
2d36079
to
8a6b394
Compare
7bd871f
to
38f2639
Compare
const compiledSubQuery = await this.source.compileQuery( | ||
const refSource = | ||
await this.source.manager.loadFromQuery(fullSubQueryPath) | ||
const compiledSubQuery = await refSource.compileQuery( |
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.
This is important. Now runQuery
is cross-source. Meaning you can call a query from another source.
const materialize = compiledSubQuery.config.materialize_query | ||
const isFalse = materialize === false | ||
|
||
if (isFalse) { |
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.
The isFalse
variable is not really needed... You can just do if (!materialize)
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.
I know but I want to let users know when they configured with false
throw new Error( | ||
`'${referencedQuery}' query can not have parameters to filter the SQL query. You're using ${usedParams} params in the query`, |
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.
Do we want to show the user which params have they used? I think that can be implicit
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.
I think it doesn't hurt no?
This commit just introduce the materialize connector but is will need another piece to work that we'll introduce in another PR. For now we don't publish this connector
38f2639
to
0da39e0
Compare
What?
We want to allow users cache queries from their sources on disk to improve performance. The queries will be stored in parquet format and read with DuckDB. For that reason, this new connector inherits from our existing DuckDB connector. This PR starts the work in this connector to make it usable in development and writing queries to disk.
Issue ticket number and link
#369
Dependency
#416
Checklist before requesting a review
Source
to connectors (done in another PR)materializedRef
function