Skip to content

Add InfluxDB Connector#24220

Open
k3rnL wants to merge 3 commits into
trinodb:masterfrom
k3rnL:k3rnl/influxdb-connector
Open

Add InfluxDB Connector#24220
k3rnL wants to merge 3 commits into
trinodb:masterfrom
k3rnL:k3rnl/influxdb-connector

Conversation

@k3rnL
Copy link
Copy Markdown

@k3rnL k3rnL commented Nov 21, 2024

Description

This PR introduces a new connector for InfluxDB, enabling Trino to query time-series data stored in InfluxDB. InfluxDB is a widely used time-series database but diverges from many traditional database standards, requiring a tailored approach to integration.

Additional context and related issues

I must note that most of the work was made in the past, but the PR was never finalized #15549
I have mostly only rebased the code, fixed tests and adapt the code to the new requirements.
My addition is the support for schema creation and schema/table destruction.

Release notes

( ) Release notes are required, with the following suggested text:

## Section
* Add InfluxDB connector. ({issue}`issuenumber`)

@github-actions github-actions Bot added the docs label Nov 21, 2024
@k3rnL k3rnL force-pushed the k3rnl/influxdb-connector branch 9 times, most recently from c26a3f8 to caed040 Compare November 22, 2024 22:30
@k3rnL
Copy link
Copy Markdown
Author

k3rnL commented Nov 23, 2024

I'm not sure if the CI fails because of the PR's code or if the tests are flaky ?

@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Nov 24, 2024

ci / pt (default, suite-all-connectors-smoke, ) looks related to this change. Other failures are unrelated.

@k3rnL k3rnL force-pushed the k3rnl/influxdb-connector branch from caed040 to 480d66d Compare November 24, 2024 21:49
@github-actions
Copy link
Copy Markdown

This pull request has gone a while without any activity. Tagging for triage help: @mosabua

@github-actions github-actions Bot added the stale label Dec 16, 2024
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 6, 2025

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

@github-actions github-actions Bot closed this Jan 6, 2025
@diegocareaga
Copy link
Copy Markdown

Hey @k3rnL Do you know what happened at the end? I will be happy to collaborate with you. I see your connector really interesting. 👍

@k3rnL
Copy link
Copy Markdown
Author

k3rnL commented Feb 17, 2025

A lack of time mostly ! The connector is working well, the problem is to be compliant with the autonomous tests..
I was close to pass all the tests though, I think it does not need a lot of work to pass the tests.

@k3rnL k3rnL reopened this Feb 17, 2025
@k3rnL k3rnL force-pushed the k3rnl/influxdb-connector branch from 480d66d to 42a0cb4 Compare February 17, 2025 20:20
@cla-bot cla-bot Bot added the cla-signed label Feb 17, 2025
@diegocareaga
Copy link
Copy Markdown

@k3rnL Could you please update the pom.xml in (plugin/trino-influxdb) with this version of trino-root snapshot? and try again? 471

471-SNAPSHOT instead of 466-SNAPSHOT and then we will see what happen

@ebyhr ebyhr mentioned this pull request Feb 18, 2025
@ebyhr ebyhr force-pushed the k3rnl/influxdb-connector branch from 42a0cb4 to b19ea18 Compare February 18, 2025 01:12
@diegocareaga
Copy link
Copy Markdown

Hi @k3rnL
I noticed two small things we can fix:

  1. Your pom.xml needs a version update - just change 466-SNAPSHOT to match current version (471-SNAPSHOT). @ebyhr helped us with that but I don't know what happened - maybe a problem with cache here? becase the outdated file is only here -> /home/runner/work/trino/trino/plugin/trino-influxdb/pom.xml

  2. There are some test failures in trino-loki module (not in your influxdb connector). Maybe we could update to the latest version and rebuild?

Really excited to try your InfluxDB connector! I'm eager to collaborate once I have access. Let me know if you need help with these fixes 😊

@ebyhr ebyhr force-pushed the k3rnl/influxdb-connector branch from b19ea18 to e30e1f4 Compare February 18, 2025 10:12
@k3rnL
Copy link
Copy Markdown
Author

k3rnL commented Feb 18, 2025

Seems @ebyhr just outperformed us 😁 Thank ! But I see that the merging is blocked because of a merge commit, but it seems this commit comes from the main branch itself. I may be mistaken, but I carefully rebased the code..

@diegocareaga
Copy link
Copy Markdown

@k3rnL congrats you already passed all the tests. Do you know what is the next step? if not I will ask in the slack channel

@k3rnL
Copy link
Copy Markdown
Author

k3rnL commented Feb 18, 2025

Perfect ! That was faster than I thought :)
Yes you may ask, thanks for your help :)

@k3rnL k3rnL force-pushed the k3rnl/influxdb-connector branch 2 times, most recently from ef05025 to 8f110cf Compare August 13, 2025 00:03
@k3rnL k3rnL force-pushed the k3rnl/influxdb-connector branch from 8f110cf to 8e6dfdc Compare August 13, 2025 00:12
Copy link
Copy Markdown
Author

@k3rnL k3rnL left a comment

Choose a reason for hiding this comment

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

I've made the corrections following your comments

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 3, 2025

This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack.

@github-actions github-actions Bot added the stale label Sep 3, 2025
@k3rnL
Copy link
Copy Markdown
Author

k3rnL commented Sep 4, 2025

@ebyhr Is there something more I can do ?

@github-actions github-actions Bot removed the stale label Sep 5, 2025
@github-actions
Copy link
Copy Markdown

This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack.

@github-actions github-actions Bot added the stale label Sep 26, 2025
@github-actions
Copy link
Copy Markdown

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

@github-actions github-actions Bot closed this Oct 17, 2025
@k3rnL k3rnL reopened this Oct 17, 2025
@github-actions github-actions Bot removed the stale label Oct 20, 2025
@dlandz
Copy link
Copy Markdown

dlandz commented Nov 5, 2025

@ebyhr can we get this reviewed again?
@k3rnL thanks for your work on this, does it need to be re-merged post release 478?

@github-actions
Copy link
Copy Markdown

This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack.

@github-actions github-actions Bot added the stale label Nov 26, 2025
}
}
catch (Exception e) {
if (this.dockerContainer != null) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: move this.dockerContainer = before try and remove the condition

}
catch (Exception e) {
if (this.dockerContainer != null) {
this.dockerContainer.close();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use closeAllSuppress


public String getEndpoint()
{
return this.dockerContainer.getUrl();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this. is redundant, please remove (& elsewhere)

@github-actions github-actions Bot removed the stale label Nov 27, 2025
Comment on lines +120 to +124
@AssertFalse(message = "'influx.username' and 'influx.password' must be both empty or both non-empty.")
public boolean isValidPasswordConfig()
{
return (username == null) ^ (password == null);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The check seems wrong

Comment on lines +129 to +134
boolean hasCreds = (username != null) || (password != null);
if (!hasCreds || endpoint == null) {
return true;
}
String scheme = URI.create(endpoint).getScheme();
return insecureAllowed || "https".equalsIgnoreCase(scheme);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you reorder the check like:

if (endpoint == null) {
....
}
// otherwise 
...

{
InfluxTableHandle tableHandle = (InfluxTableHandle) handle;
if (client.findTableHandle(tableHandle.schemaName(), tableHandle.tableName()).isEmpty()) {
throw new TableNotFoundException(tableHandle.toSchemaTableName());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure it is needed, engine should potentially check the case

Comment on lines +19 to +21
public record InfluxRecord(List<String> columns, List<List<Object>> values)
{
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
public record InfluxRecord(List<String> columns, List<List<Object>> values)
{
}
public record InfluxRecord(List<String> columns, List<List<Object>> values)
{}

if (value instanceof Number number) {
return number.intValue() != 0;
}
return false;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is the influxDB says other types value except boolea/number is treated as "false" ?

if (value instanceof Number number) {
return number.longValue();
}
return 0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here, why not throw exception

}
else {
ImmutableList.Builder<String> columns = ImmutableList.builder();
List<List<Object>> values = new ArrayList<>();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use ImmutableList

@github-actions
Copy link
Copy Markdown

This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack.

@github-actions github-actions Bot added the stale label Dec 19, 2025
@mosabua mosabua added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Dec 19, 2025
@mosabua
Copy link
Copy Markdown
Member

mosabua commented Dec 19, 2025

Since @colebow is now at InfluxData I am assigning this PR to him. @k3rnL please work with him on addressing the PR feedback and any further discussions here or on Trino slack.

I am assuming you will want to drive this PR towards merge and have therefore given it the stale-ignore label to avoid further noise.

@k3rnL
Copy link
Copy Markdown
Author

k3rnL commented Dec 20, 2025

Alright, I have not planned to work on it until beginning of the next year, I have no time for it unfortunately. But it will be a pleasure to work with you @colebow in the beginning of 2026.

I'm definitely motivated to have this PR closed, time is the only issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed docs stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed.

Development

Successfully merging this pull request may close these issues.

10 participants