Influx Connector #2397

williame · 2020-01-03T09:17:49Z

Influx is a noSQL time-series database. https://www.influxdata.com/

…e dependency problems

…ckstyle!

.gitignore

presto-influx/src/test/java/io/prestosql/plugin/influx/InfluxPluginTest.java

presto-influx/src/test/java/io/prestosql/plugin/influx/InfluxQueryTest.java

ebyhr

Left only basic comments. This is codestyle guidleline in this project if you haven't yet read it. https://github.com/prestosql/presto#code-style

presto-influx/src/test/java/io/prestosql/plugin/influx/TestingInfluxServer.java

presto-influx/src/test/java/io/prestosql/plugin/influx/TestInfluxQL.java

presto-influx/src/main/java/io/prestosql/plugin/influx/InfluxHttp.java

ebyhr · 2020-01-08T16:06:31Z

Before starting more detailed reviews, let's talk if we really want to use okhttp instead of the existing library

williame · 2020-01-08T16:41:39Z

Before starting more detailed reviews, let's talk if we really want to use okhttp instead of the existing library

https://github.com/influxdata/influxdb-java

https://doc.akka.io/docs/alpakka/current/influxdb.html

One issue is dependency versions. Adding this to the pom:

    <dependency>
        <groupId>org.influxdb</groupId>
        <artifactId>influxdb-java</artifactId>
        <version>2.17</version>
    </dependency>

Will cause okhttp3 version conflicts with the rest of presto. How would we best solve that?

The other issue is how to build the query string. At the moment, this connector in the MR is building that manually with a custom InfluxQL class. However, obviously it would smell less if we could use the QueryBuilder offered by the influx java lib instead.

However, the QueryBuilder https://github.com/influxdata/influxdb-java/blob/master/QUERY_BUILDER.md doesn't support column names that need quoting. Deep in the implementation the columns are appended to a StringBuilder unescaped in https://github.com/influxdata/influxdb-java/blob/master/src/main/java/org/influxdb/querybuilder/Appender.java#L76 and the API doc examples rely on and abuse this so it probably can't ever be added.

Another issue is the quoting of parameters. Newer versions of the influx java library do support ? to denote a parameter, rather like JDBC's PreparedStatement. However, the influx java library always passes these parameters to the influx server, even though early versions of influx didn't support this feature.

So, if we sort out the dependency problem, it looks like we'd still be building our own InfluxQL strings manually rather than using a tried and tested QueryBuilder.

ebyhr

We may want to switch to the official library later, but I left initial comments for http client.
I suppose influxdb is the exected name (≠influx) as mongodb. This is not a huge problem, but it will affect config properties.

presto-influx/pom.xml

ebyhr · 2020-01-08T13:34:58Z

presto-influx/src/main/java/io/prestosql/plugin/influx/InfluxClient.java

+    private final OkHttpClient httpClient;
+    private final String baseUrl;
+    // the various metadata are cached for a configurable number of milliseconds so we don't hammer the server
+    private final CachedMetaData<Map<String, String>> retentionPolicies;  // schema name (lower-case) -> retention policy (case-sensitive)


Basically, we use guava for cache. You can see the existing code in MongoSession

While I think it worth to cache table definitions, I'm not sure if we should store schema and table names. Are those operations slow in InfluxDB?

ebyhr · 2020-01-08T13:48:09Z

presto-influx/src/main/java/io/prestosql/plugin/influx/InfluxClient.java

+    // Influx tracks the tags in each measurement, but not which retention-policy they are used in
+    private Map<String, InfluxColumn> getTags(String tableName)
+    {
+        return tagKeys.computeIfAbsent(tableName,


This method and getFields are too nested. Please try to avoid it.

ebyhr · 2020-01-08T13:51:57Z

presto-influx/src/main/java/io/prestosql/plugin/influx/InfluxClient.java

+
+    public List<InfluxColumn> getColumns(String schemaName, String tableName)
+    {
+        List<InfluxColumn> columns = InfluxTpchTestSupport.getColumns(config.getDatabase(), schemaName, tableName);


I understood this is the temporally hack to run TPCH test. Please correct me if my understanding is wrong.

Cassandra connector has the same behavior, the column orders is arranged automatically. Then, we added extra information to hold the expected orders. Could you try adding the same feature? You can refer CassandraSession class.

ebyhr · 2020-01-08T14:01:41Z

presto-influx/src/main/java/io/prestosql/plugin/influx/InfluxClient.java

+        if (series == null) {
+            return Collections.emptyMap();
+        }
+        InfluxError.GENERAL.check(series.getNodeType().equals(JsonNodeType.ARRAY), "expecting an array, not " + series, query);


Please avoid using InfluxError check and fail methods. We can use checkArgument method, PrestoException class and so on.

ebyhr · 2020-01-22T03:01:22Z

presto-influx/src/test/java/io/prestosql/plugin/influx/InfluxQueryRunner.java

+    public InfluxQueryRunner()
+            throws Exception
+    {
+        dockerContainer = new InfluxDBContainer()


Please extract dockerContainer from this class. You can refer to TestingMySqlServer class.

ebyhr · 2020-01-22T03:04:00Z

presto-influx/src/test/java/io/prestosql/plugin/influx/TestInfluxPlugin.java

+        ConnectorFactory factory = getOnlyElement(plugin.getConnectorFactories());
+        assertInstanceOf(factory, InfluxConnectorFactory.class);
+
+        Connector c = factory.create(


Please avoid abbreviations.

Suggested change

Connector c = factory.create(

Connector connector = factory.create(

ebyhr · 2020-01-22T03:06:21Z

presto-influx/src/test/java/io/prestosql/plugin/influx/TestInfluxQL.java

+
+        // from is a reserved word so must be quoted
+        test.addIdentifier("frOm").append(" = ").add("to");
+        assertEquals(test.toString(), "\"frOm\" = 'to'");


Could you add more test cases?

ebyhr · 2020-01-22T03:13:36Z

presto-influx/src/main/java/io/prestosql/plugin/influx/InfluxClient.java

+
+public class InfluxClient
+{
+    final Logger logger;


Please make it private static final

ebyhr · 2020-01-22T03:14:13Z

presto-docs/src/main/sphinx/connector/influx.rst

+``influx.database=``                                The database name must be specified.  Each instance of the connector
+                                                    can only connect to a single database on a server
+``influx.username=``                                Optional
+``influx.password=``                                Option


Co-Authored-By: Yuya Ebihara <ebyhry@gmail.com>

ofekby · 2020-02-28T09:36:15Z

Hi is this pull request is still on the going?

RonC-LL · 2020-04-17T15:53:02Z

I subscribed to this pull request but I just wanted to echo @ofekby 's enthusiasm and interest in the final product. Thank you for your work. (I'd maybe try building what you have, but I'd rather wait until it's official.)

RonC-LL · 2020-05-07T03:33:36Z

FWIW, I built an image from Will's source - currently at Presto version 330-SNAPSHOT - and it works beautifully.

I hope this can get past the finish line and merged into the official image. It's a great connector.

UPDATE (13-May-2020): In my development environment, I forked a version of the PR, merged it with Presto 333, added some logging (the query & url strings), and added a new influx.check-collision boolean config that tells the connector whether or not it should throw an exception if it detects an Influx field name "collision" (field names are the same when lower-cased).

hrishy · 2020-06-28T05:51:13Z

Hi

Is this connector for influxdb available for download ?

deepak-k-meesho · 2020-09-22T10:38:29Z

Hi

By when will this connector be officially available?

StalkerOne · 2022-05-15T06:05:08Z

@williame hello, Does connector support influxdb now?

bitsondatadev · 2022-10-07T18:56:46Z

👋🏻 This PR looks really interesting. It looks like there's a lot of demand for it. @williame, are you still interested in working on this?

@cpard, any interest in looking into this feature?

Unfortunately, it looks like this is inactive and nobody is working on it. We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

williame added 12 commits December 18, 2019 21:52

Does metadata; queries return a fake row, without any clause push-down

3ceba4d

Basic implementation of Influx connector, using old influx lib

6644d71

Sort out what measurements are populated in which retention policies

bb80718

Timestamps now UTC; support LIMIT

aff0f88

Removed dependency on InfluxDB client libs, as they caused mvn packag…

baae62e

…e dependency problems

Okay, using the Airlift codestyle and it *still* doesn't pass mvn che…

9da4232

…ckstyle!

Fighting the style checkers

7ed8720

If a column filter cannot be handled completely, don't handle it at all

b4a89c8

Some comments; don't generate comparisons for strings

4edc8d4

Added some tests

d761c8b

Influx SQL tests with QueryRunner

0c879fa

Tidied up influx query tests

f83a5a0

cla-bot bot added the cla-signed label Jan 3, 2020

kokosing reviewed Jan 3, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

presto-influx/src/test/java/io/prestosql/plugin/influx/InfluxPluginTest.java Outdated Show resolved Hide resolved

presto-influx/src/test/java/io/prestosql/plugin/influx/InfluxQueryTest.java Outdated Show resolved Hide resolved

Use influxdb in a test-container rather than canned responses

18bce36

ebyhr reviewed Jan 5, 2020

View reviewed changes

presto-influx/src/test/java/io/prestosql/plugin/influx/TestingInfluxServer.java Outdated Show resolved Hide resolved

ebyhr reviewed Jan 5, 2020

View reviewed changes

presto-influx/src/test/java/io/prestosql/plugin/influx/TestingInfluxServer.java Outdated Show resolved Hide resolved

presto-influx/src/test/java/io/prestosql/plugin/influx/TestingInfluxServer.java Outdated Show resolved Hide resolved

ebyhr reviewed Jan 5, 2020

View reviewed changes

presto-influx/src/test/java/io/prestosql/plugin/influx/TestInfluxQL.java Outdated Show resolved Hide resolved

ebyhr reviewed Jan 5, 2020

View reviewed changes

presto-influx/src/main/java/io/prestosql/plugin/influx/InfluxHttp.java Outdated Show resolved Hide resolved

ebyhr reviewed Jan 5, 2020

View reviewed changes

presto-influx/src/main/java/io/prestosql/plugin/influx/InfluxHttp.java Outdated Show resolved Hide resolved

williame added 4 commits January 5, 2020 19:51

Tidied up tests re review

7069d3c

Influx: add integer field

1e0440b

Runs TCPH tests

aad3801

Updated Influx documentation

b5569f6

ebyhr reviewed Jan 22, 2020

View reviewed changes

williame and others added 2 commits January 22, 2020 07:11

Update presto-influx/pom.xml change version to 329

f98ef79

Co-Authored-By: Yuya Ebihara <ebyhry@gmail.com>

Merge branch 'master' into will_edwards_dev_1

7d84a4b

Merge presto master

bafcaa2

ebyhr assigned williame Mar 20, 2020

tooptoop4 mentioned this pull request Apr 18, 2020

More https://trino.io/resources.html #3472

Closed

miguelhrocha mentioned this pull request Jun 29, 2021

InfluxDB connector #8421

Open

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:14

mosabua closed this Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Influx Connector #2397

Influx Connector #2397

williame commented Jan 3, 2020

ebyhr left a comment

ebyhr commented Jan 8, 2020

williame commented Jan 8, 2020

ebyhr left a comment •

edited

Loading

ebyhr Jan 8, 2020

ebyhr Jan 8, 2020

ebyhr Jan 8, 2020 •

edited

Loading

ebyhr Jan 8, 2020

ebyhr Jan 22, 2020

ebyhr Jan 22, 2020

ebyhr Jan 22, 2020

ebyhr Jan 22, 2020

ebyhr Jan 22, 2020

ofekby commented Feb 28, 2020

RonC-LL commented Apr 17, 2020

RonC-LL commented May 7, 2020 •

edited

Loading

hrishy commented Jun 28, 2020

deepak-k-meesho commented Sep 22, 2020

StalkerOne commented May 15, 2022

bitsondatadev commented Oct 7, 2022

	Connector c = factory.create(
	Connector connector = factory.create(

Influx Connector #2397

Influx Connector #2397

Conversation

williame commented Jan 3, 2020

ebyhr left a comment

Choose a reason for hiding this comment

ebyhr commented Jan 8, 2020

williame commented Jan 8, 2020

ebyhr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofekby commented Feb 28, 2020

RonC-LL commented Apr 17, 2020

RonC-LL commented May 7, 2020 • edited Loading

hrishy commented Jun 28, 2020

deepak-k-meesho commented Sep 22, 2020

StalkerOne commented May 15, 2022

bitsondatadev commented Oct 7, 2022

ebyhr left a comment •

edited

Loading

ebyhr Jan 8, 2020 •

edited

Loading

RonC-LL commented May 7, 2020 •

edited

Loading