Skip to content

Conversation

@igorlukanin
Copy link
Member

@igorlukanin igorlukanin commented Feb 11, 2025

Check List

  • Tests have been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Issue Reference this PR resolves

Requested in Slack: https://cubedevinc.slack.com/archives/C06FQNWFL7N/p1738790583282389?thread_ts=1730929644.841189&cid=C06FQNWFL7N

@igorlukanin igorlukanin requested a review from a team as a code owner February 11, 2025 13:56
@igorlukanin igorlukanin requested review from KSDaemon and removed request for a team February 11, 2025 13:57
Copy link
Member

@KSDaemon KSDaemon left a comment

Choose a reason for hiding this comment

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

👍🏻 LGTM!

body: JSON.stringify({
sql: query,
queryOptions: `useMultistageEngine=true;timeoutMs=${this.config.queryTimeout}`
queryOptions: `useMultistageEngine=true;enableNullHandling=true;timeoutMs=${this.config.queryTimeout}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @igorlukanin

Great, the configuration is correct. Is there a possibility to add this same setting as an environment variable?
Where its default value is false?

For example:

enableNullHandling=${this.config.dbNullHandling};  

In: cube/packages/cubejs-pinot-driver/src/PinotDriver.ts

queryTimeout: getEnv('dbNullHandling', { dataSource }),  

So that I can set this variable like this:

CUBEJS_DS_PINOT_DB_NULL_HANDLING=true

Copy link
Member Author

Choose a reason for hiding this comment

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

@sergisulca I think this a great idea. Would you be open to contribute that? You can use this PR as a reference: #8744

Copy link
Contributor

Choose a reason for hiding this comment

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

I contributed this in #9213 pull request.

…nv var (#9213)

* Add enableNullHandling=true to query options using env var CUBEJS_DB_NULL_HANDLING

* Add 'The Startree/Pinot null value support' to docs

* fix reference env var

* Update docs/pages/product/configuration/data-sources/pinot.mdx

Co-authored-by: Konstantin Burkalev <[email protected]>

* Update docs/pages/reference/configuration/environment-variables.mdx

Co-authored-by: ron-damon <[email protected]>

* Update docs/pages/reference/configuration/environment-variables.mdx

Co-authored-by: ron-damon <[email protected]>

* Update packages/cubejs-backend-shared/src/env.ts

Co-authored-by: Konstantin Burkalev <[email protected]>

* fix env var name

---------

Co-authored-by: Konstantin Burkalev <[email protected]>
Co-authored-by: ron-damon <[email protected]>
@igorlukanin
Copy link
Member Author

Superseded by #9213

@sergisulca
Copy link
Contributor

Hi @igorlukanin , could it be that the changes were not merged into master?

Since the Pull Request #9213 was applied to the branch of this PR (pinot-driver-addl-option).

KSDaemon merged 8 commits into cube-js:pinot-driver-addl-option from MuttData:pinot-driver-addl-option

@KSDaemon KSDaemon deleted the pinot-driver-addl-option branch February 13, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants