Skip to content
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: add precision flag to write subcommand #25935

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

waynr
Copy link
Contributor

@waynr waynr commented Jan 30, 2025

Addresses #25930

This adds simple --precision flag to the influxdb3 write subcommand along with round trip write/query tests to validate the resulting timestamp precision in the query output.

@waynr waynr changed the title feat: add precision option to write subcommand feat: add precision flag to write subcommand Jan 30, 2025
@waynr waynr requested a review from a team January 30, 2025 01:22
Copy link
Contributor

@praveen-influx praveen-influx left a comment

Choose a reason for hiding this comment

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

Looks good.

@@ -903,6 +906,21 @@ pub enum Precision {
Nanosecond,
}

impl std::str::FromStr for Precision {
Copy link
Member

Choose a reason for hiding this comment

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

Can we re-use whatever it is that parses the precision argument from the HTTP API? That way they're always the exact same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Several types are duplicated in this crate, e.g., Format, from the influxdb3_server crate. Perhaps we should pull influxdb3_server as a dependency of influxdb3_client and re-use them directly, or move the types to a central crate.

But this has bit us a couple times now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into the type used for the HTTP API since my initial grep for an appropriate type turned up both, but it looks to me like the HTTP API uses serde deserialization where the HTTP request body refers to the precision types as "second", "millisecond", etc (as opposed to "s", "ms", etc).

Even if we wanted to use the full word for precision unit for the CLI, we would still need to impl FromStr, which is what clap uses to parse arguments.

@hiltontj I did notice the TODOs in the code around consolidating types between crates -- is there a github issue tracking that? I could look into that after this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@waynr waynr merged commit bb92eb0 into main Jan 30, 2025
14 checks passed
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.

4 participants