siddharthteotia commented on a change in pull request #6710:
URL: https://github.com/apache/incubator-pinot/pull/6710#discussion_r599312636
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/common/datatable/DataTableImplV2V3.java
##########
@@ -33,12 +33,15 @@
import org.apache.pinot.common.utils.DataTable;
import org.apache.pinot.common.utils.StringUtil;
import org.apache.pinot.core.common.ObjectSerDeUtils;
+import org.apache.pinot.core.query.request.context.ThreadTimer;
import org.apache.pinot.spi.utils.ByteArray;
import org.apache.pinot.spi.utils.BytesUtils;
-public class DataTableImplV2 implements DataTable {
- private static final int VERSION = 2;
+public class DataTableImplV2V3 implements DataTable {
Review comment:
Let's discuss the approach again by moving the metadata to the end of
the payload. I think we both are inclined towards doing that since all the
metadata (existing + new) will be together in the footer.
Coming to naming, my initial suggestion of not including version was indeed
because they share the logic. So tomorrow if we move to v4 and still share a
lot of common logic, we can continue to retain the name DataTableImpl and not
DataTableImplv2v3v4 as everything will be in the same file as long as it is
readable.
I agree that moving the metadata is a change which will make some code
unreadable if we try to keep everything in the same file. So yes, if we go down
this path, I agree we should create a new class.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
