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 csv influx v1 #25030

Merged

Conversation

JeanArhancet
Copy link
Contributor

@JeanArhancet JeanArhancet commented May 30, 2024

This PR is Part of #24770

Goal: Extend current v1 implementation to support CSV format.

  • Added QueryFormat enum to handle CSV, JSON, and pretty JSON formats.
  • Refactored conversion of QueryResponse to Bytes to include CSV support.

Check-list:

  • I've read the contributing section of the project README.
  • Signed CLA (if not already signed).

@hiltontj
Copy link
Contributor

Hey @JeanArhancet - thanks for opening the PR! If it is still WIP, then I suggest converting it to Draft. Then, once you're happy with it, you can flip it back to open and request review from myself or one of the other maintainers.

Also, CI runs cargo commands (clippy, fmt, test, etc.) on the latest version of Rust, so it may help to have the latest version in your development environment as well, i.e., run

rustup update stable

Looks like you need to check clippy and are also getting an audit failure. The latter can usually be fixed with a cargo update from the root of the repository.

@pauldix
Copy link
Member

pauldix commented May 31, 2024

The v1 API of InfluxDB only supports JSON and JSON Pretty. For other formats, we've added the /api/v3/query_influxql endpoint which can return in JSON (not the same format as the v1 JSON), JSONL, Pretty Print (useful for printing to a terminal), CSV, and Parquet.

I'd prefer not to add anything new to the v1 API. It's there for backward compatibility to help people upgrading. But for new functionality, users will have to move to the v3 APIs.

@pauldix
Copy link
Member

pauldix commented May 31, 2024

Actually, @hiltontj just let me know that we already support pretty in the v1 API. So I think we can close this out in favor of users hitting /api/v3/query_influxql for alternative format.

Thanks for pitching in @JeanArhancet. We're still very early and haven't been taking outside contributions so we haven't ironed out that process yet. We'll update our CONTRIBUTING with a little explanation. The high level is that people should always start with an open issue if they want to contribute. If no issue exists for what they want to add, opening an issue is the best starting point so that we can comment there before implementation.

@pauldix pauldix closed this May 31, 2024
@pauldix
Copy link
Member

pauldix commented May 31, 2024

Ah, @hiltontj just now reminded me that v1 does in fact support CSV. I had forgotten and when I took a look at the docs before closing out this PR, I couldn't find mention of it. However, it's here: https://docs.influxdata.com/influxdb/v1/tools/api/#request-query-results-in-csv-format.

And the issue we had open was here: #24770.

So I'll re-open, assuming you're trying to support that format. If not, and you're just looking for InfluxQL and csv results, v3 is probably what you want.

@pauldix pauldix reopened this May 31, 2024
@JeanArhancet JeanArhancet force-pushed the feat/add-application-csv-influx-v1 branch from 24da08b to 0e57fd8 Compare June 1, 2024 12:39
@JeanArhancet JeanArhancet force-pushed the feat/add-application-csv-influx-v1 branch 2 times, most recently from cf52cc0 to df1be6f Compare June 1, 2024 12:49
@JeanArhancet JeanArhancet force-pushed the feat/add-application-csv-influx-v1 branch from df1be6f to df70e21 Compare June 1, 2024 13:12
@JeanArhancet
Copy link
Contributor Author

Thanks for your feedback, @hiltontj . The PR is ready to be reviewed.

@pauldix Yes, the goal of this MR is to cover the CSV format as in Influx v1.

@JeanArhancet JeanArhancet changed the title [WIP] feat: add csv influx v1 feat: add csv influx v1 Jun 1, 2024
@hiltontj hiltontj added the v3 label Jun 3, 2024
Copy link
Contributor

@hiltontj hiltontj left a comment

Choose a reason for hiding this comment

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

I have a few comments in line for you to take a look at before approving.

influxdb3_server/src/http/v1.rs Outdated Show resolved Hide resolved
influxdb3_server/src/http/v1.rs Outdated Show resolved Hide resolved
influxdb3_server/src/http/v1.rs Show resolved Hide resolved
influxdb3_server/src/http/v1.rs Outdated Show resolved Hide resolved
@JeanArhancet JeanArhancet force-pushed the feat/add-application-csv-influx-v1 branch from 66806a8 to 4dea77c Compare June 20, 2024 06:37
@JeanArhancet
Copy link
Contributor Author

Sorry for the delay, @hiltontj . I have applied your feedback and also fixed the code and added integration tests for this part. What do you think?

@JeanArhancet JeanArhancet force-pushed the feat/add-application-csv-influx-v1 branch from 4dea77c to ba00dfc Compare June 20, 2024 06:52
@hiltontj
Copy link
Contributor

@JeanArhancet I will try to get some time today or tomorrow to take a look at this, but in the meantime, CI has a cargo-audit failure, which can usually be resolved by doing a cargo update and then commit/push.

Copy link
Contributor

@hiltontj hiltontj left a comment

Choose a reason for hiding this comment

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

@JeanArhancet - it's looking great! I left one minor nit in a comment but otherwise LGTM, thanks for sticking with it.

influxdb3_server/src/http/v1.rs Show resolved Hide resolved
influxdb3_server/src/http/v1.rs Show resolved Hide resolved
@hiltontj hiltontj merged commit b6718e5 into influxdata:main Jun 25, 2024
6 checks passed
@hiltontj
Copy link
Contributor

Merged! Thank you @JeanArhancet, again, for your PR 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants