You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "cecemei (via GitHub)" <gi...@apache.org> on 2025/07/09 03:00:51 UTC
[PR] Add projection names to DataSegment in a backward compatible manner. (druid)
cecemei opened a new pull request, #18223:
URL: https://github.com/apache/druid/pull/18223
### Description
This PR adds an optional projectionNames field to DataSegment to support tracking projection-specific metadata at the segment level.
##### Key changed/added classes in this PR
* `DataSegment`
* `DataSegmentTest`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2193927860
##########
processing/src/main/java/org/apache/druid/timeline/DataSegment.java:
##########
@@ -471,17 +468,27 @@ public String toString()
", loadSpec=" + loadSpec +
", dimensions=" + dimensions +
", metrics=" + metrics +
+ ", projections=" + projections +
", shardSpec=" + shardSpec +
", lastCompactionState=" + lastCompactionState +
", size=" + size +
'}';
}
+ /**
+ * @deprecated use {@link #builder(SegmentId)} or {@link #builder(DataSegment)} instead.
+ */
+ @Deprecated
Review Comment:
I don't think we should deprecate this. It is useful (mostly in tests) when building a segment from scratch.
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2208120787
##########
sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java:
##########
@@ -545,7 +545,7 @@ public void testGetTableMap()
final RelDataType rowType = segmentsTable.getRowType(new JavaTypeFactoryImpl());
final List<RelDataTypeField> fields = rowType.getFieldList();
- Assert.assertEquals(19, fields.size());
+ Assert.assertEquals(20, fields.size());
Review Comment:
added projection test in segment1 and publishedUncompactedSegment3
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2205547803
##########
processing/src/main/java/org/apache/druid/timeline/DataSegment.java:
##########
@@ -230,7 +246,7 @@ public DataSegment(
}
@Nullable
- private Map<String, Object> prepareLoadSpec(@Nullable Map<String, Object> loadSpec)
+ private static Map<String, Object> prepareLoadSpec(@Nullable Map<String, Object> loadSpec)
Review Comment:
nit: if you have to make any additional changes to this PR, maybe move these static methods to be after the instance methods. this is just my personal preference, so not a blocker or anything
##########
sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java:
##########
@@ -545,7 +545,7 @@ public void testGetTableMap()
final RelDataType rowType = segmentsTable.getRowType(new JavaTypeFactoryImpl());
final List<RelDataTypeField> fields = rowType.getFieldList();
- Assert.assertEquals(19, fields.size());
+ Assert.assertEquals(20, fields.size());
Review Comment:
maybe we could add a test for a table with projections?
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2193927860
##########
processing/src/main/java/org/apache/druid/timeline/DataSegment.java:
##########
@@ -471,17 +468,27 @@ public String toString()
", loadSpec=" + loadSpec +
", dimensions=" + dimensions +
", metrics=" + metrics +
+ ", projections=" + projections +
", shardSpec=" + shardSpec +
", lastCompactionState=" + lastCompactionState +
", size=" + size +
'}';
}
+ /**
+ * @deprecated use {@link #builder(SegmentId)} or {@link #builder(DataSegment)} instead.
+ */
+ @Deprecated
Review Comment:
I don't think we should deprecate this. It is useful (mostly in tests) when building a segment from scratch.
##########
processing/src/main/java/org/apache/druid/timeline/DataSegment.java:
##########
@@ -495,20 +502,36 @@ public static class Builder
private Map<String, Object> loadSpec;
private List<String> dimensions;
private List<String> metrics;
+ private List<String> projections;
private ShardSpec shardSpec;
private CompactionState lastCompactionState;
private Integer binaryVersion;
private long size;
+ /**
+ * @deprecated use {@link #Builder(SegmentId)} or {@link #Builder(DataSegment)} instead.
+ */
+ @Deprecated
public Builder()
Review Comment:
If this is not used anywhere, we can probably just make this constructor private.
##########
server/src/test/java/org/apache/druid/client/ImmutableDruidDataSourceTest.java:
##########
@@ -38,15 +39,21 @@
public class ImmutableDruidDataSourceTest
{
+ DataSegment TEST_SEGMENT = DataSegment.builder(SegmentId.of("test", Intervals.of("2017/2018"), "version", null))
Review Comment:
static final
##########
processing/src/main/java/org/apache/druid/timeline/DataSegment.java:
##########
@@ -156,13 +167,19 @@ public DataSegment(
loadSpec,
dimensions,
metrics,
+ null,
shardSpec,
null,
binaryVersion,
- size
+ size,
+ PruneSpecsHolder.DEFAULT
);
}
+ /**
+ * @deprecated use {@link #builder(SegmentId)} or {@link #builder(DataSegment)} instead.
+ */
+ @Deprecated
Review Comment:
It seems that this PR is removing some usages of these constructors.
If there are no more usages left, we can just remove this constructor or make it private.
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1203,18 +1203,12 @@ private SegmentPublishResult commitAppendSegmentsAndMetadataInTransaction(
final SegmentId newVersionSegmentId = pendingSegment.getId().asSegmentId();
newVersionSegmentToParent.put(newVersionSegmentId, oldSegment.getId());
upgradedFromSegmentIdMap.put(newVersionSegmentId.toString(), oldSegment.getId().toString());
- allSegmentsToInsert.add(
- new DataSegment(
- pendingSegment.getId().asSegmentId(),
- oldSegment.getLoadSpec(),
- oldSegment.getDimensions(),
- oldSegment.getMetrics(),
- pendingSegment.getId().getShardSpec(),
- oldSegment.getLastCompactionState(),
- oldSegment.getBinaryVersion(),
- oldSegment.getSize()
- )
- );
+ allSegmentsToInsert.add(DataSegment.builder(oldSegment)
+ .dataSource(newVersionSegmentId.getDataSource())
Review Comment:
This can be removed as the `dataSource` wouldn't change from the `oldSegment`.
##########
processing/src/main/java/org/apache/druid/timeline/DataSegment.java:
##########
@@ -98,6 +99,7 @@ public static class PruneSpecsHolder
private final Map<String, Object> loadSpec;
private final List<String> dimensions;
private final List<String> metrics;
+ private final List<String> projections;
Review Comment:
Did you intend to name this field `projectionNames` as mentioned in the PR description?
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2208097168
##########
processing/src/main/java/org/apache/druid/timeline/DataSegment.java:
##########
@@ -156,13 +167,19 @@ public DataSegment(
loadSpec,
dimensions,
metrics,
+ null,
shardSpec,
null,
binaryVersion,
- size
+ size,
+ PruneSpecsHolder.DEFAULT
);
}
+ /**
+ * @deprecated use {@link #builder(SegmentId)} or {@link #builder(DataSegment)} instead.
+ */
+ @Deprecated
Review Comment:
These constructors are used in many places, and I don't believe this PR removes all of them. I'm marking them as deprecated so we can remove them gradually as we encounter them.
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2196386347
##########
server/src/test/java/org/apache/druid/segment/indexing/DataSchemaTest.java:
##########
@@ -582,30 +585,73 @@
DataSchema.class
);
- Assert.assertEquals(actual.getDataSource(), IdUtilsTest.VALID_ID_CHARS);
+ Assert.assertEquals(IdUtilsTest.VALID_ID_CHARS, actual.getDataSource());
Assert.assertEquals(
- actual.getParser().getParseSpec(),
new JSONParseSpec(
new TimestampSpec("xXx", null, null),
DimensionsSpec.builder().setDimensionExclusions(Arrays.asList("__time", "metric1", "xXx", "col1")).build(),
null,
null,
null
- )
+ ),
+ actual.getParser().getParseSpec()
);
Assert.assertArrayEquals(
- actual.getAggregators(),
new AggregatorFactory[]{
new DoubleSumAggregatorFactory("metric1", "col1")
- }
+ },
+ actual.getAggregators()
);
Assert.assertEquals(
- actual.getGranularitySpec(),
new ArbitraryGranularitySpec(
new DurationGranularity(86400000, null),
ImmutableList.of(Intervals.of("2014/2015"))
- )
+ ),
+ actual.getGranularitySpec()
+ );
+ Assert.assertNull(actual.getProjections());
+ }
+
+ @Test
+ public void testSerdeWithProjections() throws Exception
+ {
+ // serialize, then deserialize of DataSchema with projections.
+ JSONParseSpec parseSpec = new JSONParseSpec(
+ new TimestampSpec("time", "auto", null),
+ new DimensionsSpec(DimensionsSpec.getDefaultSchemas(ImmutableList.of(
+ "dimB",
+ "dimA"
+ ))).withDimensionExclusions(ImmutableSet.of("__time", "time")),
+ null,
+ null,
+ null
+ );
+ AggregateProjectionSpec projectionSpec = new AggregateProjectionSpec(
+ "ab_count_projection",
+ null,
+ Arrays.asList(
+ new StringDimensionSchema("a"),
+ new LongDimensionSchema("b")
+ ),
+ new AggregatorFactory[]{
+ new CountAggregatorFactory("count")
+ }
);
+ DataSchema original = DataSchema.builder()
+ .withDataSource("datasource")
+ .withObjectMapper(jsonMapper)
+ .withParserMap(jsonMapper.convertValue(
+ new StringInputRowParser(parseSpec, null),
+ JacksonUtils.TYPE_REFERENCE_MAP_STRING_OBJECT
+ ))
Review Comment:
## Deprecated method or constructor invocation
Invoking [Builder.withParserMap](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/9395)
##########
server/src/test/java/org/apache/druid/segment/indexing/DataSchemaTest.java:
##########
@@ -582,30 +585,73 @@
DataSchema.class
);
- Assert.assertEquals(actual.getDataSource(), IdUtilsTest.VALID_ID_CHARS);
+ Assert.assertEquals(IdUtilsTest.VALID_ID_CHARS, actual.getDataSource());
Assert.assertEquals(
- actual.getParser().getParseSpec(),
new JSONParseSpec(
new TimestampSpec("xXx", null, null),
DimensionsSpec.builder().setDimensionExclusions(Arrays.asList("__time", "metric1", "xXx", "col1")).build(),
null,
null,
null
- )
+ ),
+ actual.getParser().getParseSpec()
);
Assert.assertArrayEquals(
- actual.getAggregators(),
new AggregatorFactory[]{
new DoubleSumAggregatorFactory("metric1", "col1")
- }
+ },
+ actual.getAggregators()
);
Assert.assertEquals(
- actual.getGranularitySpec(),
new ArbitraryGranularitySpec(
new DurationGranularity(86400000, null),
ImmutableList.of(Intervals.of("2014/2015"))
- )
+ ),
+ actual.getGranularitySpec()
+ );
+ Assert.assertNull(actual.getProjections());
+ }
+
+ @Test
+ public void testSerdeWithProjections() throws Exception
+ {
+ // serialize, then deserialize of DataSchema with projections.
+ JSONParseSpec parseSpec = new JSONParseSpec(
+ new TimestampSpec("time", "auto", null),
+ new DimensionsSpec(DimensionsSpec.getDefaultSchemas(ImmutableList.of(
+ "dimB",
+ "dimA"
+ ))).withDimensionExclusions(ImmutableSet.of("__time", "time")),
+ null,
+ null,
+ null
+ );
+ AggregateProjectionSpec projectionSpec = new AggregateProjectionSpec(
+ "ab_count_projection",
+ null,
+ Arrays.asList(
+ new StringDimensionSchema("a"),
+ new LongDimensionSchema("b")
+ ),
+ new AggregatorFactory[]{
+ new CountAggregatorFactory("count")
+ }
);
+ DataSchema original = DataSchema.builder()
+ .withDataSource("datasource")
+ .withObjectMapper(jsonMapper)
Review Comment:
## Deprecated method or constructor invocation
Invoking [Builder.withObjectMapper](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/9396)
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2209307111
##########
sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java:
##########
@@ -583,138 +577,104 @@ public void testSegmentsTable() throws Exception
rows.sort((Object[] row1, Object[] row2) -> ((Comparable) row1[0]).compareTo(row2[0]));
// total segments = 8
- // segments test1, test2 are published and available
- // segment test3 is served by historical but unpublished or unused
- // segments test4, test5 are not published but available (realtime segments)
- // segment test2 is both published and served by a realtime server.
-
Assert.assertEquals(8, rows.size());
-
- verifyRow(
- rows.get(0),
- "test1_2010-01-01T00:00:00.000Z_2011-01-01T00:00:00.000Z_version1",
- 100L,
- 0L, //partition_num
- 1L, //num_replicas
- 3L, //numRows
- 1L, //is_published
- 1L, //is_available
- 0L, //is_realtime
- 1L, //is_overshadowed
- null, //is_compacted
- 2L // replication_factor
- );
-
- verifyRow(
- rows.get(1),
- "test2_2011-01-01T00:00:00.000Z_2012-01-01T00:00:00.000Z_version2",
- 100L,
- 0L, //partition_num
- 2L, //x§segment test2 is served by historical and realtime servers
- 3L, //numRows
- 1L, //is_published
- 1L, //is_available
- 0L, //is_realtime
- 0L, //is_overshadowed,
- null, //is_compacted
- 0L // replication_factor
- );
-
- //segment test3 is unpublished and has a NumberedShardSpec with partitionNum = 2
- verifyRow(
- rows.get(2),
- "test3_2012-01-01T00:00:00.000Z_2013-01-01T00:00:00.000Z_version3_2",
- 100L,
- 2L, //partition_num
- 1L, //num_replicas
- 2L, //numRows
- 0L, //is_published
- 1L, //is_available
- 0L, //is_realtime
- 0L, //is_overshadowed
- null, //is_compacted
- -1L // replication_factor
- );
-
- verifyRow(
- rows.get(3),
- "test4_2014-01-01T00:00:00.000Z_2015-01-01T00:00:00.000Z_version4",
- 100L,
- 0L, //partition_num
- 1L, //num_replicas
- 0L, //numRows
- 0L, //is_published
- 1L, //is_available
- 1L, //is_realtime
- 0L, //is_overshadowed
- null, //is_compacted
- -1L // replication_factor
- );
-
- verifyRow(
- rows.get(4),
- "test5_2015-01-01T00:00:00.000Z_2016-01-01T00:00:00.000Z_version5",
- 100L,
- 0L, //partition_num
- 1L, //num_replicas
- 0L, //numRows
- 0L, //is_published
- 1L, //is_available
- 1L, //is_realtime
- 0L, //is_overshadowed
- null, //is_compacted
- -1L // replication_factor
- );
-
- // wikipedia segments are published and unavailable, num_replicas is 0
- // wikipedia segment 1 and 2 are compacted while 3 are not compacted
- verifyRow(
- rows.get(5),
- "wikipedia1_2007-01-01T00:00:00.000Z_2008-01-01T00:00:00.000Z_version1",
- 53000L,
- 0L, //partition_num
- 0L, //num_replicas
- 0L, //numRows
- 1L, //is_published
- 0L, //is_available
- 0L, //is_realtime
- 1L, //is_overshadowed
- expectedCompactionState, //is_compacted
- 2L // replication_factor
- );
-
- verifyRow(
- rows.get(6),
- "wikipedia2_2008-01-01T00:00:00.000Z_2009-01-01T00:00:00.000Z_version2",
- 83000L,
- 0L, //partition_num
- 0L, //num_replicas
- 0L, //numRows
- 1L, //is_published
- 0L, //is_available
- 0L, //is_realtime
- 0L, //is_overshadowed
- expectedCompactionState, //is_compacted
- 0L // replication_factor
- );
-
- verifyRow(
- rows.get(7),
- "wikipedia3_2009-01-01T00:00:00.000Z_2010-01-01T00:00:00.000Z_version3",
- 47000L,
- 0L, //partition_num
- 0L, //num_replicas
- 0L, //numRows
- 1L, //is_published
- 0L, //is_available
- 0L, //is_realtime
- 0L, //is_overshadowed
- null, //is_compacted
- 2L // replication_factor
- );
-
// Verify value types.
verifyTypes(rows, SystemSchema.SEGMENTS_SIGNATURE);
+
+ // segments test1, test2 are published and available.
+ Object[] segment1Expected = new Object[]{
Review Comment:
nit: when we have to split declarations across rows we usually do 1 entry per line (except for map key/value pairs)
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2208133263
##########
processing/src/main/java/org/apache/druid/timeline/DataSegment.java:
##########
@@ -98,6 +99,7 @@ public static class PruneSpecsHolder
private final Map<String, Object> loadSpec;
private final List<String> dimensions;
private final List<String> metrics;
+ private final List<String> projections;
Review Comment:
It's somewhat implied that it's just projection names, similar to how metrics and dimensions are listed by name, not by their full definitions.
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1203,18 +1203,12 @@ private SegmentPublishResult commitAppendSegmentsAndMetadataInTransaction(
final SegmentId newVersionSegmentId = pendingSegment.getId().asSegmentId();
newVersionSegmentToParent.put(newVersionSegmentId, oldSegment.getId());
upgradedFromSegmentIdMap.put(newVersionSegmentId.toString(), oldSegment.getId().toString());
- allSegmentsToInsert.add(
- new DataSegment(
- pendingSegment.getId().asSegmentId(),
- oldSegment.getLoadSpec(),
- oldSegment.getDimensions(),
- oldSegment.getMetrics(),
- pendingSegment.getId().getShardSpec(),
- oldSegment.getLastCompactionState(),
- oldSegment.getBinaryVersion(),
- oldSegment.getSize()
- )
- );
+ allSegmentsToInsert.add(DataSegment.builder(oldSegment)
+ .dataSource(newVersionSegmentId.getDataSource())
Review Comment:
true, removed
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2208137271
##########
processing/src/main/java/org/apache/druid/timeline/DataSegment.java:
##########
@@ -230,7 +246,7 @@ public DataSegment(
}
@Nullable
- private Map<String, Object> prepareLoadSpec(@Nullable Map<String, Object> loadSpec)
+ private static Map<String, Object> prepareLoadSpec(@Nullable Map<String, Object> loadSpec)
Review Comment:
updated this
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2208134837
##########
processing/src/main/java/org/apache/druid/timeline/DataSegment.java:
##########
@@ -495,20 +502,36 @@ public static class Builder
private Map<String, Object> loadSpec;
private List<String> dimensions;
private List<String> metrics;
+ private List<String> projections;
private ShardSpec shardSpec;
private CompactionState lastCompactionState;
private Integer binaryVersion;
private long size;
+ /**
+ * @deprecated use {@link #Builder(SegmentId)} or {@link #Builder(DataSegment)} instead.
+ */
+ @Deprecated
public Builder()
Review Comment:
true, updated to be private
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2208133843
##########
server/src/test/java/org/apache/druid/client/ImmutableDruidDataSourceTest.java:
##########
@@ -38,15 +39,21 @@
public class ImmutableDruidDataSourceTest
{
+ DataSegment TEST_SEGMENT = DataSegment.builder(SegmentId.of("test", Intervals.of("2017/2018"), "version", null))
Review Comment:
true updated
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei merged PR #18223:
URL: https://github.com/apache/druid/pull/18223
--
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 projection names to DataSegment in a backward compatible manner. (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18223:
URL: https://github.com/apache/druid/pull/18223#discussion_r2210909055
##########
sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java:
##########
@@ -583,138 +577,104 @@ public void testSegmentsTable() throws Exception
rows.sort((Object[] row1, Object[] row2) -> ((Comparable) row1[0]).compareTo(row2[0]));
// total segments = 8
- // segments test1, test2 are published and available
- // segment test3 is served by historical but unpublished or unused
- // segments test4, test5 are not published but available (realtime segments)
- // segment test2 is both published and served by a realtime server.
-
Assert.assertEquals(8, rows.size());
-
- verifyRow(
- rows.get(0),
- "test1_2010-01-01T00:00:00.000Z_2011-01-01T00:00:00.000Z_version1",
- 100L,
- 0L, //partition_num
- 1L, //num_replicas
- 3L, //numRows
- 1L, //is_published
- 1L, //is_available
- 0L, //is_realtime
- 1L, //is_overshadowed
- null, //is_compacted
- 2L // replication_factor
- );
-
- verifyRow(
- rows.get(1),
- "test2_2011-01-01T00:00:00.000Z_2012-01-01T00:00:00.000Z_version2",
- 100L,
- 0L, //partition_num
- 2L, //x§segment test2 is served by historical and realtime servers
- 3L, //numRows
- 1L, //is_published
- 1L, //is_available
- 0L, //is_realtime
- 0L, //is_overshadowed,
- null, //is_compacted
- 0L // replication_factor
- );
-
- //segment test3 is unpublished and has a NumberedShardSpec with partitionNum = 2
- verifyRow(
- rows.get(2),
- "test3_2012-01-01T00:00:00.000Z_2013-01-01T00:00:00.000Z_version3_2",
- 100L,
- 2L, //partition_num
- 1L, //num_replicas
- 2L, //numRows
- 0L, //is_published
- 1L, //is_available
- 0L, //is_realtime
- 0L, //is_overshadowed
- null, //is_compacted
- -1L // replication_factor
- );
-
- verifyRow(
- rows.get(3),
- "test4_2014-01-01T00:00:00.000Z_2015-01-01T00:00:00.000Z_version4",
- 100L,
- 0L, //partition_num
- 1L, //num_replicas
- 0L, //numRows
- 0L, //is_published
- 1L, //is_available
- 1L, //is_realtime
- 0L, //is_overshadowed
- null, //is_compacted
- -1L // replication_factor
- );
-
- verifyRow(
- rows.get(4),
- "test5_2015-01-01T00:00:00.000Z_2016-01-01T00:00:00.000Z_version5",
- 100L,
- 0L, //partition_num
- 1L, //num_replicas
- 0L, //numRows
- 0L, //is_published
- 1L, //is_available
- 1L, //is_realtime
- 0L, //is_overshadowed
- null, //is_compacted
- -1L // replication_factor
- );
-
- // wikipedia segments are published and unavailable, num_replicas is 0
- // wikipedia segment 1 and 2 are compacted while 3 are not compacted
- verifyRow(
- rows.get(5),
- "wikipedia1_2007-01-01T00:00:00.000Z_2008-01-01T00:00:00.000Z_version1",
- 53000L,
- 0L, //partition_num
- 0L, //num_replicas
- 0L, //numRows
- 1L, //is_published
- 0L, //is_available
- 0L, //is_realtime
- 1L, //is_overshadowed
- expectedCompactionState, //is_compacted
- 2L // replication_factor
- );
-
- verifyRow(
- rows.get(6),
- "wikipedia2_2008-01-01T00:00:00.000Z_2009-01-01T00:00:00.000Z_version2",
- 83000L,
- 0L, //partition_num
- 0L, //num_replicas
- 0L, //numRows
- 1L, //is_published
- 0L, //is_available
- 0L, //is_realtime
- 0L, //is_overshadowed
- expectedCompactionState, //is_compacted
- 0L // replication_factor
- );
-
- verifyRow(
- rows.get(7),
- "wikipedia3_2009-01-01T00:00:00.000Z_2010-01-01T00:00:00.000Z_version3",
- 47000L,
- 0L, //partition_num
- 0L, //num_replicas
- 0L, //numRows
- 1L, //is_published
- 0L, //is_available
- 0L, //is_realtime
- 0L, //is_overshadowed
- null, //is_compacted
- 2L // replication_factor
- );
-
// Verify value types.
verifyTypes(rows, SystemSchema.SEGMENTS_SIGNATURE);
+
+ // segments test1, test2 are published and available.
+ Object[] segment1Expected = new Object[]{
Review Comment:
There're 20 elements and it feels noisy and visually bloated when 1 entry per line. Compact formatting looks better for readability in this case IMO.
--
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