Skip to content

Conversation

RaphDal
Copy link

@RaphDal RaphDal commented Sep 24, 2025

This PR adds the documentation to use the decimal type in QuestDB.

Related PR: #6068

TODO

Optional

  • Concept page

Copy link

github-actions bot commented Sep 24, 2025

🚀 Build success!

Latest successful preview: https://preview-242--questdb-documentation.netlify.app/docs/

Commit SHA: 0632722

📦 Build generates a preview & updates link on each commit.

@javier javier self-requested a review October 8, 2025 09:11
Copy link
Contributor

@javier javier left a comment

Choose a reason for hiding this comment

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

A few comments, but super solid docs!


- `nanRate` is an `int` defining the frequency of occurrence of `NaN` values:
- `0`: No `NaN` will be returned.
- `1`: Will only return `NaN`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not factual! I think it has been broken for a while also on other rnd generators as not the first time I see this. But if you execute with 1 you will see it returns only some nan, not all

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably this should be fixed in code, rather than on docs

Copy link
Author

Choose a reason for hiding this comment

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

@puzpuzpuz I think the problem comes from the + 1 here, do you happen to know why it's there?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not aware of the initial reason to approach null rate like this in the random functions, but changing this would be a breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

It would be a breaking change for other types, the decimal type hasn't been released yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only pending point on my end to approve. If you agree this is not a decimal issue, but a problem with the current implementation, I am happy to approve

Copy link
Author

Choose a reason for hiding this comment

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

The documentation doesn't make sense with the current implementation, we should change one of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I am just not sure if the current behaviour is something we want to document, as I am not sure what it does

Copy link
Contributor

Choose a reason for hiding this comment

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

guess we can just approve, seeing as this is a wider problem and not related to decimal. The day (if any) it gets fixed for all other types, it will be fixed for decimal as well

@javier javier marked this pull request as ready for review October 14, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants