gortiz commented on code in PR #13303:
URL: https://github.com/apache/pinot/pull/13303#discussion_r1738768777
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/DataBlock.java:
##########
@@ -104,4 +113,39 @@ public static Type fromOrdinal(int ordinal) {
}
}
}
+
+ /**
+ * Returns the dictionary for the given column.
+ *
+ * This is a break in the interface abstraction that assumes all
implementations will use a dictionary only for
+ * string columns. This may change in the future.
+ */
+ @Nullable
+ String[] getStringDictionary();
+
+ /**
+ * The actual content is different depending on whether this is a row-based
or columnar data block.
+ *
+ * This is an abstraction leak that assumes all implementations derive from
{@link BaseDataBlock}.
Review Comment:
This interface doesn't make much sense. It is an _old_ interface and it
should be treated as a sealed class where all instances are well known at
compile time.
Therefore yes, we assume that the only 4 classes that will implement this
interface are:
1. BaseDataBlock
2. MetaDataBlock
3. RowDataBlock
4. ColumnDataBlock
If we had the time to rewrite these classes I'm sure we would use a
different names and hierarchy today. Even the name of the interface is
incorrect, given that MetaDataBlock does not actually carry any data.
As a history note, in the past we had another concept called NOOP which I
don't remember if it was another class or just another type of MetaDataBlock.
NOOP blocks were used to implement our own coroutine model and therefore do not
need to use CachedThreadPool as we do right now. We ended up giving up on that
because that leaked into our Operator implementations and it was very easy to
make mistakes and forget about NOOP. IMHO the concept of NOOP wasn't that
problematic. The issue was the actual implementation we have here in DataBlock.
I'm sure with some rewrites we would be able to have a DataBlock hierarchy that
was type safe (so we couldn't forget about NOOP).
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
