You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2024/09/17 03:59:12 UTC
[PR] add VirtualColumns.findEquivalent and VirtualColumn.isEquivalent (druid)
clintropolis opened a new pull request, #17084:
URL: https://github.com/apache/druid/pull/17084
Adds methods to make it easier to check for equivalent virtual columns in the 'native' engine layer, even in cases where output names differ. This method isn't really used yet, but has some use cases in follow-up PRs.
I have only implemented this method for `ExpressionVirtualColumn`, `NestedFieldVirtualColumn`, and `ListFilteredVirtualColumn` so far, and really only needed it for `ExpressionVirtualColumn`. The default implementation falls back to using equals, though I noticed a handful of implementations (`FallbackVirtualColumn`, `HashPartitionVirtualColumn`, and `SettableLongVirtualColumn`) do not implement that either. Given their use-cases, and the intended use-case of this method, I don't think it probably matters, though it should be easy enough to add them if we need it.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] add VirtualColumns.findEquivalent and VirtualColumn.isEquivalent (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17084:
URL: https://github.com/apache/druid/pull/17084#discussion_r1762846718
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumn.java:
##########
@@ -336,4 +336,12 @@ default ColumnIndexSupplier getIndexSupplier(
{
return NoIndexesColumnIndexSupplier.getInstance();
}
+
+ /**
+ * Check if a virtual column is the same as some other virtual column, ignoring output name.
+ */
+ default boolean isEquivalent(VirtualColumn other)
+ {
+ return this.equals(other);
+ }
Review Comment:
leaving it like this could still have adverse effects as if a descendant doesn't override equals it will fall back to `Object.equals`
I think it would be better to annotate `VirtualColumn` with `@SubclassesMustOverrideEqualsAndHashCode` and fix the issues.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] add VirtualColumns.findEquivalent and VirtualColumn.EquivalenceKey (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #17084:
URL: https://github.com/apache/druid/pull/17084
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] add VirtualColumns.findEquivalent and VirtualColumn.isEquivalent (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17084:
URL: https://github.com/apache/druid/pull/17084#discussion_r1762655462
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumn.java:
##########
@@ -336,4 +336,12 @@ default ColumnIndexSupplier getIndexSupplier(
{
return NoIndexesColumnIndexSupplier.getInstance();
}
+
+ /**
+ * Check if a virtual column is the same as some other virtual column, ignoring output name.
+ */
+ default boolean isEquivalent(VirtualColumn other)
+ {
+ return this.equals(other);
+ }
Review Comment:
stuff probably should implement hashcode + equals anyway, but I'm not really comfortable at this time changing the contract of equals and hashcode to not include the output name, since it is more of a behavioral change than I was really looking to make at this time.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] add VirtualColumns.findEquivalent and VirtualColumn.isEquivalent (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17084:
URL: https://github.com/apache/druid/pull/17084#discussion_r1762645130
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumns.java:
##########
@@ -172,10 +174,28 @@ public VirtualColumn getVirtualColumn(String columnName)
if (vc != null) {
return vc;
}
+ if (hasNoDotColumns) {
+ return null;
+ }
final String baseColumnName = splitColumnName(columnName).lhs;
return withDotSupport.get(baseColumnName);
}
+ /**
+ * Check if a virtual column is already defined which is the same as some other virtual column, ignoring output name,
+ * returning that virtual column if it exists, or null if there is no equivalent virtual column.
+ */
+ @Nullable
+ public VirtualColumn findEquivalent(VirtualColumn virtualColumn)
Review Comment:
perhaps the context of why I am adding this method is missing. I'm working on a feature called projections, which allows transformed and pre-aggregated data to exist on top of a base segment, and automatically be used by a query against the base. Part of the needs of this is to match up virtual columns present in the query to those present in the projections, so if a table exists that already contains the transform we can use it and map projected virtual column to the query virtual column to skip having to compute it. I don't actually want to add this virtual column to `VirtualColumns`, though I should probably precompute an equivalence set or something to use for better lookup.
The SQL layer currently handles deduplication of expressions before creating VirtualColumns with `VirtualColumnRegistry`, so there isn't a super strong need to have the same stuff here.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] add VirtualColumns.findEquivalent and VirtualColumn.isEquivalent (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17084:
URL: https://github.com/apache/druid/pull/17084#discussion_r1762536527
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumns.java:
##########
@@ -172,10 +174,28 @@ public VirtualColumn getVirtualColumn(String columnName)
if (vc != null) {
return vc;
}
+ if (hasNoDotColumns) {
+ return null;
+ }
final String baseColumnName = splitColumnName(columnName).lhs;
return withDotSupport.get(baseColumnName);
}
+ /**
+ * Check if a virtual column is already defined which is the same as some other virtual column, ignoring output name,
+ * returning that virtual column if it exists, or null if there is no equivalent virtual column.
+ */
+ @Nullable
+ public VirtualColumn findEquivalent(VirtualColumn virtualColumn)
Review Comment:
why there is a need to expose this directly?
wouldn't it be better to make it the internal detail/contract of `VirtualColumns` that any instance will make sure to reuse existing `VC` columns?
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumn.java:
##########
@@ -336,4 +336,12 @@ default ColumnIndexSupplier getIndexSupplier(
{
return NoIndexesColumnIndexSupplier.getInstance();
}
+
+ /**
+ * Check if a virtual column is the same as some other virtual column, ignoring output name.
+ */
+ default boolean isEquivalent(VirtualColumn other)
+ {
+ return this.equals(other);
+ }
Review Comment:
I don't think this is a good move; you are adding a method which will resort to object level compare (as default implementation).
why not make sure to implement hashcode + equals?
another pro of using equals is that it has the same signature ; and its a base method
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumns.java:
##########
@@ -172,10 +174,28 @@ public VirtualColumn getVirtualColumn(String columnName)
if (vc != null) {
return vc;
}
+ if (hasNoDotColumns) {
+ return null;
+ }
final String baseColumnName = splitColumnName(columnName).lhs;
return withDotSupport.get(baseColumnName);
}
+ /**
+ * Check if a virtual column is already defined which is the same as some other virtual column, ignoring output name,
+ * returning that virtual column if it exists, or null if there is no equivalent virtual column.
+ */
+ @Nullable
+ public VirtualColumn findEquivalent(VirtualColumn virtualColumn)
+ {
+ for (VirtualColumn vc : virtualColumns) {
+ if (vc.isEquivalent(virtualColumn)) {
Review Comment:
wouldn't this this for loop will cost `O(n^2)` if it gets run for all VC-s in the query;
I think another pro of making it an internal contract/detail could enable the use of a `Set` so that it doesn't involve much cost
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] add VirtualColumns.findEquivalent and VirtualColumn.isEquivalent (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17084:
URL: https://github.com/apache/druid/pull/17084#discussion_r1762846718
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumn.java:
##########
@@ -336,4 +336,12 @@ default ColumnIndexSupplier getIndexSupplier(
{
return NoIndexesColumnIndexSupplier.getInstance();
}
+
+ /**
+ * Check if a virtual column is the same as some other virtual column, ignoring output name.
+ */
+ default boolean isEquivalent(VirtualColumn other)
+ {
+ return this.equals(other);
+ }
Review Comment:
leaving it like this could still have adverse effects as if a descendant doesn't override equals it will fall back to `Object.equals`
I think it would be better to annotate `VirtualColumn` with `@SubclassesMustOverrideEqualsAndHashCode` and fix the issues.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org