imply-cheddar commented on code in PR #12392:
URL: https://github.com/apache/druid/pull/12392#discussion_r849980931
##########
indexing-service/src/main/java/org/apache/druid/indexing/input/DruidSegmentInputFormat.java:
##########
@@ -55,14 +56,30 @@ public InputEntityReader createReader(
File temporaryDirectory
)
{
- return new DruidSegmentReader(
- source,
- indexIO,
- inputRowSchema.getTimestampSpec(),
- inputRowSchema.getDimensionsSpec(),
- inputRowSchema.getColumnsFilter(),
- dimFilter,
- temporaryDirectory
+ // this method handles the case when the entity comes from a tombstone or
from a regular segment
+ Preconditions.checkArgument(
+ source instanceof DruidSegmentInputEntity,
+ DruidSegmentInputEntity.class.getName() + " required, but "
+ + source.getClass().getName() + " provided."
);
+
+ InputEntityReader retVal = null;
+ // source is tombstone case
+ if ((source instanceof DruidSegmentInputEntity) &&
((DruidSegmentInputEntity) source).isFromTombstone()) {
Review Comment:
nit: you are basically doing this same instanceof twice now (one for the
precondition and one here, might as well just make the pre-condition an `if`
statement and then cast and store a method-local variable.
##########
indexing-service/src/main/java/org/apache/druid/indexing/input/DruidSegmentInputFormat.java:
##########
@@ -55,14 +56,30 @@ public InputEntityReader createReader(
File temporaryDirectory
)
{
- return new DruidSegmentReader(
- source,
- indexIO,
- inputRowSchema.getTimestampSpec(),
- inputRowSchema.getDimensionsSpec(),
- inputRowSchema.getColumnsFilter(),
- dimFilter,
- temporaryDirectory
+ // this method handles the case when the entity comes from a tombstone or
from a regular segment
+ Preconditions.checkArgument(
+ source instanceof DruidSegmentInputEntity,
+ DruidSegmentInputEntity.class.getName() + " required, but "
+ + source.getClass().getName() + " provided."
);
+
+ InputEntityReader retVal = null;
+ // source is tombstone case
+ if ((source instanceof DruidSegmentInputEntity) &&
((DruidSegmentInputEntity) source).isFromTombstone()) {
+ retVal = new DruidTombstoneSegmentReader(source);
+ }
+ // source is not tombstone:
+ if (retVal == null) {
Review Comment:
Checking for `retVal == null` instead of just an `else` seems weird?
##########
indexing-service/src/main/java/org/apache/druid/indexing/input/DruidSegmentReader.java:
##########
@@ -70,9 +68,8 @@
import java.util.NoSuchElementException;
import java.util.Set;
-public class DruidSegmentReader extends
IntermediateRowParsingReader<Map<String, Object>>
+public class DruidSegmentReader extends DruidSegmentReaderBase
Review Comment:
Design nit: I'm not sure that this parent class is really adding much. You
haven't been able to delete much code from this class, it looks like all you
are doing is delegating the storage of `source`. For the TombstoneReader, it's
also not really doing that much. Seems like it might even be less code to have
them just be separate classes without any attempt at inheritance.
--
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]
