You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "ImprovingRichard (via GitHub)" <gi...@apache.org> on 2023/02/16 17:17:52 UTC
[GitHub] [pinot] ImprovingRichard opened a new pull request, #10292: [feature] Add jdbc multistage support
ImprovingRichard opened a new pull request, #10292:
URL: https://github.com/apache/pinot/pull/10292
Updated PinotConnection.java to support passing useMultistageEngine in the connection string which sets the option per query.
Refactored how connection string options are processed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112036218
##########
pinot-clients/pinot-jdbc-client/src/test/java/org/apache/pinot/client/PinotPreparedStatementTest.java:
##########
@@ -120,4 +123,30 @@ public void testSetAdditionalDataTypes()
Assert.assertEquals(lastExecutedQuery.substring(0, lastExecutedQuery.indexOf("LIMIT")).trim(),
String.format("SELECT * FROM dummy WHERE value = '%s'", Hex.encodeHexString(value.getBytes())));
}
+
+ @Test
+ public void testSetEnableNullHandling()
+ throws Exception {
+ Properties props = new Properties();
+ props.put(QueryOptionKey.ENABLE_NULL_HANDLING, "true");
+ PinotConnection pinotConnection =
+ new PinotConnection(props, "dummy", _dummyPinotClientTransport, "dummy", _dummyPinotControllerTransport);
+ PreparedStatement preparedStatement = pinotConnection.prepareStatement(BASIC_TEST_QUERY);
+ preparedStatement.executeQuery();
+ String expectedSql = DriverUtils.createSetQueryOptionString(QueryOptionKey.ENABLE_NULL_HANDLING) + BASIC_TEST_QUERY;
+ Assert.assertEquals(expectedSql, _dummyPinotClientTransport.getLastQuery().substring(0, expectedSql.length()));
+ }
+
+ @Test
+ public void testSetEnableNullHandling2()
+ throws Exception {
+ Properties props = new Properties();
+ props.put(QueryOptionKey.ENABLE_NULL_HANDLING, "true");
+ PinotConnection pinotConnection =
+ new PinotConnection(props, "dummy", _dummyPinotClientTransport, "dummy", _dummyPinotControllerTransport);
+ PreparedStatement preparedStatement = pinotConnection.prepareStatement("");
+ preparedStatement.executeQuery(BASIC_TEST_QUERY);
Review Comment:
I just separated the tests as they are different code paths. Can you explain the concern with having them separated?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1110621495
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
Review Comment:
Maybe directly get a map of query option from `properties`, and you can use it for any future supported query options as well.
For backward compatibility, you can set `enableNullHandling` if configured as `properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING)`
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
+ _queryOptions.put(possibleQueryOption, Boolean.parseBoolean(properties.getProperty(possibleQueryOption)));
+ }
}
public org.apache.pinot.client.Connection getSession() {
return _session;
}
- public boolean isNullHandlingEnabled() {
- return _enableNullHandling;
+ protected String enableQueryOptions(String sql) {
+ StringBuilder optionsBuilder = new StringBuilder();
+ for (HashMap.Entry<String, Boolean> optionEntry: _queryOptions.entrySet()) {
+ if (optionEntry.getValue() && !sql.contains(optionEntry.getKey())) {
+ optionsBuilder.append(DriverUtils.createSetQueryOptionString(optionEntry.getKey()));
Review Comment:
Should append a kv pair.
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -38,11 +40,16 @@
public class PinotConnection extends AbstractBaseConnection {
private static final Logger LOGGER = LoggerFactory.getLogger(Connection.class);
+ protected static final String[] POSSIBLE_QUERY_OPTIONS = {
+ QueryOptionKey.ENABLE_NULL_HANDLING,
+ QueryOptionKey.USE_MULTISTAGE_ENGINE
+ };
private org.apache.pinot.client.Connection _session;
private boolean _closed;
private String _controllerURL;
private PinotControllerTransport _controllerTransport;
- private final boolean _enableNullHandling;
+ private final HashMap<String, Boolean> _queryOptions = new HashMap<String, Boolean>();
Review Comment:
use `Map<String, String> _queryOptions` for future extensibility.
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
+ _queryOptions.put(possibleQueryOption, Boolean.parseBoolean(properties.getProperty(possibleQueryOption)));
+ }
}
public org.apache.pinot.client.Connection getSession() {
return _session;
}
- public boolean isNullHandlingEnabled() {
- return _enableNullHandling;
+ protected String enableQueryOptions(String sql) {
Review Comment:
just return query options and make a static method in `DriverUtils` to take both sql and query option map to reconstruct the query.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112030426
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -38,11 +40,16 @@
public class PinotConnection extends AbstractBaseConnection {
private static final Logger LOGGER = LoggerFactory.getLogger(Connection.class);
+ protected static final String[] POSSIBLE_QUERY_OPTIONS = {
+ QueryOptionKey.ENABLE_NULL_HANDLING,
+ QueryOptionKey.USE_MULTISTAGE_ENGINE
+ };
private org.apache.pinot.client.Connection _session;
private boolean _closed;
private String _controllerURL;
private PinotControllerTransport _controllerTransport;
- private final boolean _enableNullHandling;
+ private final HashMap<String, Boolean> _queryOptions = new HashMap<String, Boolean>();
Review Comment:
WIll do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1113955987
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -38,11 +40,16 @@
public class PinotConnection extends AbstractBaseConnection {
private static final Logger LOGGER = LoggerFactory.getLogger(Connection.class);
+ protected static final String[] POSSIBLE_QUERY_OPTIONS = {
+ QueryOptionKey.ENABLE_NULL_HANDLING,
+ QueryOptionKey.USE_MULTISTAGE_ENGINE
+ };
private org.apache.pinot.client.Connection _session;
private boolean _closed;
private String _controllerURL;
private PinotControllerTransport _controllerTransport;
- private final boolean _enableNullHandling;
+ private final Map<String, Boolean> _queryOptions = new HashMap<String, Boolean>();
Review Comment:
I don't think the time that we could spend there would be sensible. Anyway, it seems you already changed that to support objects. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112041186
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
Review Comment:
With this implementation, to support new query options, an entry per option is added to the POSSIBLE_QUERY_OPTIONS array. All other code remains the same. It also keeps the filtering of query options to the connection creation instead of having to do it per statement execution and keeps from having to hash into the options to get the enable flag per execution.
I'm not sure how backward compatibility comes into play.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112034990
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
+ _queryOptions.put(possibleQueryOption, Boolean.parseBoolean(properties.getProperty(possibleQueryOption)));
+ }
}
public org.apache.pinot.client.Connection getSession() {
return _session;
}
- public boolean isNullHandlingEnabled() {
- return _enableNullHandling;
+ protected String enableQueryOptions(String sql) {
Review Comment:
Changed to having the option append operation in DriverUtils.
You can enable either or both options with this implementation. It's actually setup to allow any combination of the option keys in the Connection.POSSIBLE_QUERY_OPTIONS array.
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -38,11 +40,16 @@
public class PinotConnection extends AbstractBaseConnection {
private static final Logger LOGGER = LoggerFactory.getLogger(Connection.class);
+ protected static final String[] POSSIBLE_QUERY_OPTIONS = {
+ QueryOptionKey.ENABLE_NULL_HANDLING,
+ QueryOptionKey.USE_MULTISTAGE_ENGINE
+ };
private org.apache.pinot.client.Connection _session;
private boolean _closed;
private String _controllerURL;
private PinotControllerTransport _controllerTransport;
- private final boolean _enableNullHandling;
+ private final HashMap<String, Boolean> _queryOptions = new HashMap<String, Boolean>();
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr merged pull request #10292: [feature] Add jdbc multistage support
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #10292:
URL: https://github.com/apache/pinot/pull/10292
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112041186
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
Review Comment:
With this implementation, to support new query options, an entry per option is added to the POSSIBLE_QUERY_OPTIONS array. All other code remains the same. It also keeps the filtering of query options to the connection creation instead of having to do it per statement execution. It also keeps from having to hash into the options to get the enable flag per statement execution.
I'm not sure how backward compatibility comes into play.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10292: [feature] Add jdbc multistage support
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10292:
URL: https://github.com/apache/pinot/pull/10292#issuecomment-1433583729
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10292](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c3e198d) into [master](https://codecov.io/gh/apache/pinot/commit/addb6525fca86331dfc468209164bab1636289fb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (addb652) will **decrease** coverage by `6.21%`.
> The diff coverage is `78.57%`.
> :exclamation: Current head c3e198d differs from pull request most recent head ee0cca0. Consider uploading reports for the commit ee0cca0 to get more accurate results
```diff
@@ Coverage Diff @@
## master #10292 +/- ##
============================================
- Coverage 70.39% 64.19% -6.21%
- Complexity 5777 5801 +24
============================================
Files 2015 1962 -53
Lines 109318 107177 -2141
Branches 16615 16389 -226
============================================
- Hits 76954 68797 -8157
- Misses 26961 33368 +6407
+ Partials 5403 5012 -391
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `67.65% <ø> (-0.01%)` | :arrow_down: |
| unittests2 | `13.69% <78.57%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/pinot/client/utils/DriverUtils.java](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qZGJjLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L3V0aWxzL0RyaXZlclV0aWxzLmphdmE=) | `4.49% <ø> (-11.47%)` | :arrow_down: |
| [...rg/apache/pinot/client/PinotPreparedStatement.java](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qZGJjLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Bpbm90UHJlcGFyZWRTdGF0ZW1lbnQuamF2YQ==) | `51.00% <50.00%> (ø)` | |
| [.../java/org/apache/pinot/client/PinotConnection.java](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qZGJjLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Bpbm90Q29ubmVjdGlvbi5qYXZh) | `64.44% <81.81%> (-2.23%)` | :arrow_down: |
| [...n/java/org/apache/pinot/client/PinotStatement.java](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qZGJjLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Bpbm90U3RhdGVtZW50LmphdmE=) | `39.53% <100.00%> (-4.66%)` | :arrow_down: |
| [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ache/pinot/server/access/AccessControlFactory.java](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VydmVyL2FjY2Vzcy9BY2Nlc3NDb250cm9sRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ache/pinot/common/messages/ForceCommitMessage.java](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvRm9yY2VDb21taXRNZXNzYWdlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [464 more](https://codecov.io/gh/apache/pinot/pull/10292?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112034990
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
+ _queryOptions.put(possibleQueryOption, Boolean.parseBoolean(properties.getProperty(possibleQueryOption)));
+ }
}
public org.apache.pinot.client.Connection getSession() {
return _session;
}
- public boolean isNullHandlingEnabled() {
- return _enableNullHandling;
+ protected String enableQueryOptions(String sql) {
Review Comment:
Will do pushing the option append operation to DriverUtils.
You can enable either or both options with this implementation. It's actually setup to allow any combination of the option keys in the Connection.POSSIBLE_QUERY_OPTIONS array.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112037934
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotPreparedStatement.java:
##########
@@ -51,7 +51,7 @@ public PinotPreparedStatement(PinotConnection connection, String query) {
if (!DriverUtils.queryContainsLimitStatement(_query)) {
_query += " " + LIMIT_STATEMENT + " " + _maxRows;
}
- _query = DriverUtils.enableNullHandling(_connection, _query);
+ _query = _connection.enableQueryOptions(_query);
Review Comment:
V1 vs V2 is controlled by an option as well as the cluster config. QueryOptionKey.USE_MULTISTAGE_ENGINE is an existing key.
I did consider whether this is useful long term because I'm not sure if the intent is to support both versions in the future, but for our testing I need to be able to set the v2 option from the client without having to set it in the sql statement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112037934
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotPreparedStatement.java:
##########
@@ -51,7 +51,7 @@ public PinotPreparedStatement(PinotConnection connection, String query) {
if (!DriverUtils.queryContainsLimitStatement(_query)) {
_query += " " + LIMIT_STATEMENT + " " + _maxRows;
}
- _query = DriverUtils.enableNullHandling(_connection, _query);
+ _query = _connection.enableQueryOptions(_query);
Review Comment:
My understanding is that V1 vs V2 is controlled by an option as well as the cluster config. QueryOptionKey.USE_MULTISTAGE_ENGINE is an existing key.
I did consider whether this is useful long term because I'm not sure if the intent is to support both versions in the future, but for our current testing I need to be able to set the v2 option from the client without having to set it in the sql statement. Having it controlled via the subprotocol may be more difficult to configure in Tableau.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112037934
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotPreparedStatement.java:
##########
@@ -51,7 +51,7 @@ public PinotPreparedStatement(PinotConnection connection, String query) {
if (!DriverUtils.queryContainsLimitStatement(_query)) {
_query += " " + LIMIT_STATEMENT + " " + _maxRows;
}
- _query = DriverUtils.enableNullHandling(_connection, _query);
+ _query = _connection.enableQueryOptions(_query);
Review Comment:
V1 vs V2 is controlled by an option as well as the cluster config. QueryOptionKey.USE_MULTISTAGE_ENGINE is an existing key.
I did consider whether this is useful long term because I'm not sure if the intent is to support both versions in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112031843
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
+ _queryOptions.put(possibleQueryOption, Boolean.parseBoolean(properties.getProperty(possibleQueryOption)));
+ }
}
public org.apache.pinot.client.Connection getSession() {
return _session;
}
- public boolean isNullHandlingEnabled() {
- return _enableNullHandling;
+ protected String enableQueryOptions(String sql) {
+ StringBuilder optionsBuilder = new StringBuilder();
+ for (HashMap.Entry<String, Boolean> optionEntry: _queryOptions.entrySet()) {
+ if (optionEntry.getValue() && !sql.contains(optionEntry.getKey())) {
+ optionsBuilder.append(DriverUtils.createSetQueryOptionString(optionEntry.getKey()));
Review Comment:
Can you clarify? DriverUtils.createSetQueryOptionString does create a kv string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112037934
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotPreparedStatement.java:
##########
@@ -51,7 +51,7 @@ public PinotPreparedStatement(PinotConnection connection, String query) {
if (!DriverUtils.queryContainsLimitStatement(_query)) {
_query += " " + LIMIT_STATEMENT + " " + _maxRows;
}
- _query = DriverUtils.enableNullHandling(_connection, _query);
+ _query = _connection.enableQueryOptions(_query);
Review Comment:
V1 vs V2 is controlled by an option as well as the cluster config. QueryOptionKey.USE_MULTISTAGE_ENGINE is an existing key.
I did consider whether this is useful long term because I'm not sure if the intent is to support both versions in the future, but for our testing I need to be able to set the v2 option from the client without modifying the sql statement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1113498241
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -38,11 +40,16 @@
public class PinotConnection extends AbstractBaseConnection {
private static final Logger LOGGER = LoggerFactory.getLogger(Connection.class);
+ protected static final String[] POSSIBLE_QUERY_OPTIONS = {
+ QueryOptionKey.ENABLE_NULL_HANDLING,
+ QueryOptionKey.USE_MULTISTAGE_ENGINE
+ };
private org.apache.pinot.client.Connection _session;
private boolean _closed;
private String _controllerURL;
private PinotControllerTransport _controllerTransport;
- private final boolean _enableNullHandling;
+ private final Map<String, Boolean> _queryOptions = new HashMap<String, Boolean>();
Review Comment:
The current set of options we are interested in are booleans, so adding this will be for possible future needs. Adding more generic support will increase the processing time per statement execution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1113353480
##########
pinot-clients/pinot-jdbc-client/src/test/java/org/apache/pinot/client/PinotPreparedStatementTest.java:
##########
@@ -120,4 +123,30 @@ public void testSetAdditionalDataTypes()
Assert.assertEquals(lastExecutedQuery.substring(0, lastExecutedQuery.indexOf("LIMIT")).trim(),
String.format("SELECT * FROM dummy WHERE value = '%s'", Hex.encodeHexString(value.getBytes())));
}
+
+ @Test
+ public void testSetEnableNullHandling()
+ throws Exception {
+ Properties props = new Properties();
+ props.put(QueryOptionKey.ENABLE_NULL_HANDLING, "true");
+ PinotConnection pinotConnection =
+ new PinotConnection(props, "dummy", _dummyPinotClientTransport, "dummy", _dummyPinotControllerTransport);
+ PreparedStatement preparedStatement = pinotConnection.prepareStatement(BASIC_TEST_QUERY);
+ preparedStatement.executeQuery();
+ String expectedSql = DriverUtils.createSetQueryOptionString(QueryOptionKey.ENABLE_NULL_HANDLING) + BASIC_TEST_QUERY;
+ Assert.assertEquals(expectedSql, _dummyPinotClientTransport.getLastQuery().substring(0, expectedSql.length()));
+ }
+
+ @Test
+ public void testSetEnableNullHandling2()
+ throws Exception {
+ Properties props = new Properties();
+ props.put(QueryOptionKey.ENABLE_NULL_HANDLING, "true");
+ PinotConnection pinotConnection =
+ new PinotConnection(props, "dummy", _dummyPinotClientTransport, "dummy", _dummyPinotControllerTransport);
+ PreparedStatement preparedStatement = pinotConnection.prepareStatement("");
+ preparedStatement.executeQuery(BASIC_TEST_QUERY);
Review Comment:
oh no i simply was thinking if there's nothing actually being done in preparedStatement we can simplify it by testing it together with all the setups and potentially simplify the code. but we can refactor later
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1111057861
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotPreparedStatement.java:
##########
@@ -51,7 +51,7 @@ public PinotPreparedStatement(PinotConnection connection, String query) {
if (!DriverUtils.queryContainsLimitStatement(_query)) {
_query += " " + LIMIT_STATEMENT + " " + _maxRows;
}
- _query = DriverUtils.enableNullHandling(_connection, _query);
+ _query = _connection.enableQueryOptions(_query);
Review Comment:
I am a bit confused on how would the user control whether to use pinot-v1 or pinot-v2.
enable null handling is configured by option but are we planning to do so as well for v1 vs. v2?
should we create a different connection string format for v1 or v2, for example:
- for V1: `jdbc:pinot:default_database`
- for V2: `jdbc:pinot_v2:default_database`
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
+ _queryOptions.put(possibleQueryOption, Boolean.parseBoolean(properties.getProperty(possibleQueryOption)));
+ }
}
public org.apache.pinot.client.Connection getSession() {
return _session;
}
- public boolean isNullHandlingEnabled() {
- return _enableNullHandling;
+ protected String enableQueryOptions(String sql) {
Review Comment:
+1. also is there any case we want to enable null handling but not v2. or vice-versa?
##########
pinot-clients/pinot-jdbc-client/src/test/java/org/apache/pinot/client/PinotPreparedStatementTest.java:
##########
@@ -120,4 +123,30 @@ public void testSetAdditionalDataTypes()
Assert.assertEquals(lastExecutedQuery.substring(0, lastExecutedQuery.indexOf("LIMIT")).trim(),
String.format("SELECT * FROM dummy WHERE value = '%s'", Hex.encodeHexString(value.getBytes())));
}
+
+ @Test
+ public void testSetEnableNullHandling()
+ throws Exception {
+ Properties props = new Properties();
+ props.put(QueryOptionKey.ENABLE_NULL_HANDLING, "true");
+ PinotConnection pinotConnection =
+ new PinotConnection(props, "dummy", _dummyPinotClientTransport, "dummy", _dummyPinotControllerTransport);
+ PreparedStatement preparedStatement = pinotConnection.prepareStatement(BASIC_TEST_QUERY);
+ preparedStatement.executeQuery();
+ String expectedSql = DriverUtils.createSetQueryOptionString(QueryOptionKey.ENABLE_NULL_HANDLING) + BASIC_TEST_QUERY;
+ Assert.assertEquals(expectedSql, _dummyPinotClientTransport.getLastQuery().substring(0, expectedSql.length()));
+ }
+
+ @Test
+ public void testSetEnableNullHandling2()
+ throws Exception {
+ Properties props = new Properties();
+ props.put(QueryOptionKey.ENABLE_NULL_HANDLING, "true");
+ PinotConnection pinotConnection =
+ new PinotConnection(props, "dummy", _dummyPinotClientTransport, "dummy", _dummyPinotControllerTransport);
+ PreparedStatement preparedStatement = pinotConnection.prepareStatement("");
+ preparedStatement.executeQuery(BASIC_TEST_QUERY);
Review Comment:
the 2 tests is only different by the execution is whether prepared or now? can't we test them in just 1 method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on PR #10292:
URL: https://github.com/apache/pinot/pull/10292#issuecomment-1446581084
I think it's all set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112034990
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
+ _queryOptions.put(possibleQueryOption, Boolean.parseBoolean(properties.getProperty(possibleQueryOption)));
+ }
}
public org.apache.pinot.client.Connection getSession() {
return _session;
}
- public boolean isNullHandlingEnabled() {
- return _enableNullHandling;
+ protected String enableQueryOptions(String sql) {
Review Comment:
Will do pushing the option append operation to DriverUtils.
You can enable either or both option with this implementation. It's actually setup to allow any combination of the option keys in the Connection.POSSIBLE_QUERY_OPTIONS array.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112031843
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
+ _queryOptions.put(possibleQueryOption, Boolean.parseBoolean(properties.getProperty(possibleQueryOption)));
+ }
}
public org.apache.pinot.client.Connection getSession() {
return _session;
}
- public boolean isNullHandlingEnabled() {
- return _enableNullHandling;
+ protected String enableQueryOptions(String sql) {
+ StringBuilder optionsBuilder = new StringBuilder();
+ for (HashMap.Entry<String, Boolean> optionEntry: _queryOptions.entrySet()) {
+ if (optionEntry.getValue() && !sql.contains(optionEntry.getKey())) {
+ optionsBuilder.append(DriverUtils.createSetQueryOptionString(optionEntry.getKey()));
Review Comment:
Can you clarify? DriverUtils.createSetQueryOptionString does create a kv string. Do you mean switching from using a Map of options to something like a List<Pair<String, boolean>> ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112995453
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -38,11 +40,16 @@
public class PinotConnection extends AbstractBaseConnection {
private static final Logger LOGGER = LoggerFactory.getLogger(Connection.class);
+ protected static final String[] POSSIBLE_QUERY_OPTIONS = {
+ QueryOptionKey.ENABLE_NULL_HANDLING,
+ QueryOptionKey.USE_MULTISTAGE_ENGINE
+ };
private org.apache.pinot.client.Connection _session;
private boolean _closed;
private String _controllerURL;
private PinotControllerTransport _controllerTransport;
- private final boolean _enableNullHandling;
+ private final Map<String, Boolean> _queryOptions = new HashMap<String, Boolean>();
Review Comment:
I think it is nice to generalise the query options but... couldn't values be other things like ints? I would prefer to use Object as the value of the map and change `DriverUtils::createSetQueryOptionString` to be able to deal with, at least, booleans and integers. Strings would be also fine, but they I'm not sure whether they could be used to do SQL injections or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ImprovingRichard commented on a diff in pull request #10292: [feature] Add jdbc multistage support
Posted by "ImprovingRichard (via GitHub)" <gi...@apache.org>.
ImprovingRichard commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1112034990
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
}
_session = new org.apache.pinot.client.Connection(properties, brokers, transport);
- _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+ for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
+ _queryOptions.put(possibleQueryOption, Boolean.parseBoolean(properties.getProperty(possibleQueryOption)));
+ }
}
public org.apache.pinot.client.Connection getSession() {
return _session;
}
- public boolean isNullHandlingEnabled() {
- return _enableNullHandling;
+ protected String enableQueryOptions(String sql) {
Review Comment:
Will do pushing the option append operation to DriverUtils.
You can enable either or both optiona with this implementation. It's actually setup to allow any combination of the option keys in the Connection.POSSIBLE_QUERY_OPTIONS array.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org