kfaraz commented on code in PR #17353:
URL: https://github.com/apache/druid/pull/17353#discussion_r1863357224
##########
indexing-service/pom.xml:
##########
@@ -195,6 +195,22 @@
<artifactId>commons-collections4</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>com.cronutils</groupId>
+ <artifactId>cron-utils</artifactId>
+ <version>9.2.0</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.druid</groupId>
+ <artifactId>druid-sql</artifactId>
Review Comment:
No, we should definitely not add a Calcite dependency in these modules.
You might need to create some new interfaces and data model classes to make
sure that the clients are able to talk to the Broker without worrying about the
deps required by the actual implementation.
Please let me know if you need assistance with this. I guess you could pull
the required changes out into a separate PR.
The reason we should do this before we merge the changes in the current PR
is that once we add the dependency, subsequent contributions might end up using
all sorts of classes from those modules and it would become more difficult to
pull it out later.
As for the Explain, I think we should remove that for now at least, and
always force the user to pass the datasource name.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
