You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2023/03/23 23:07:13 UTC
[GitHub] [pinot] Jackie-Jiang opened a new pull request, #10468: Make column order deterministic in segment
Jackie-Jiang opened a new pull request, #10468:
URL: https://github.com/apache/pinot/pull/10468
In segment metadata and index map, store columns in alphabetical order so that the result is deterministic.
This is very important to ensure segment generated from the same data has the same CRC. When multiple servers consume from the same stream, it is critical that all segments generated has the same CRC, or server will need to download a new segment copy when restart due to CRC mismatch. This can cause significant longer server restart time.
## Release Notes
Segment generated before/after this PR will have different CRC, so during the upgrade, we might get segments with different CRC from old and new consuming server. For the segment consumed during the upgrade, some downloads might be needed.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10468: Make column order deterministic in segment
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10468:
URL: https://github.com/apache/pinot/pull/10468#discussion_r1153575353
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/SegmentMetadata.java:
##########
@@ -98,11 +100,11 @@ public interface SegmentMetadata {
String getEndOffset();
- default Set<String> getAllColumns() {
+ default NavigableSet<String> getAllColumns() {
Review Comment:
It is an ordered set, but not a `TreeSet` because the key set of the `TreeMap` is not a `TreeSet`
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #10468: Make column order deterministic in segment
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10468:
URL: https://github.com/apache/pinot/pull/10468#discussion_r1147160435
##########
pinot-core/src/test/java/org/apache/pinot/core/util/CrcUtilsTest.java:
##########
@@ -19,71 +19,43 @@
package org.apache.pinot.core.util;
import java.io.File;
+import java.net.URL;
import java.util.concurrent.TimeUnit;
import org.apache.commons.io.FileUtils;
-import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader;
import org.apache.pinot.segment.local.segment.creator.SegmentTestUtils;
-import org.apache.pinot.segment.local.segment.creator.impl.SegmentCreationDriverFactory;
+import org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl;
import org.apache.pinot.segment.local.utils.CrcUtils;
-import org.apache.pinot.segment.spi.IndexSegment;
-import org.apache.pinot.segment.spi.SegmentMetadata;
import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig;
import org.apache.pinot.segment.spi.creator.SegmentIndexCreationDriver;
-import org.apache.pinot.spi.utils.ReadMode;
import org.apache.pinot.util.TestUtils;
-import org.testng.Assert;
import org.testng.annotations.Test;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
-/**
- * Dec 4, 2014
- */
public class CrcUtilsTest {
-
+ private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "CrcUtilsTest");
private static final String AVRO_DATA = "data/test_data-mv.avro";
- private static final File INDEX_DIR = new File("/tmp/testingCrc");
+ private static final long EXPECTED_CRC = 4139425029L;
@Test
- public void test1()
+ public void testCrc()
Review Comment:
Does this test different column orders give same CRC"
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] claudeyj commented on pull request #10468: Make column order deterministic in segment
Posted by "claudeyj (via GitHub)" <gi...@apache.org>.
claudeyj commented on PR #10468:
URL: https://github.com/apache/pinot/pull/10468#issuecomment-1491025449
Thank you for fixing this! Glad to know this PR got merged!
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #10468: Make column order deterministic in segment
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10468:
URL: https://github.com/apache/pinot/pull/10468#issuecomment-1490677641
@xiangfu0 @61yao If the change is applied in-place on the server side, we don't update (re-compute) the CRC even though the segment file is changed so that server won't re-download the segment from deep store
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #10468: Make column order deterministic in segment
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10468:
URL: https://github.com/apache/pinot/pull/10468#discussion_r1152725258
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/SegmentMetadata.java:
##########
@@ -98,11 +100,11 @@ public interface SegmentMetadata {
String getEndOffset();
- default Set<String> getAllColumns() {
+ default NavigableSet<String> getAllColumns() {
Review Comment:
Shall we directly use TreeSet instead of NavigableSet if there is no other implementation expected .
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #10468: Make column order deterministic in segment
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10468:
URL: https://github.com/apache/pinot/pull/10468
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on pull request #10468: Make column order deterministic in segment
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #10468:
URL: https://github.com/apache/pinot/pull/10468#issuecomment-1489701245
A high level question, will this handle the scenario that during schema evolution, a new column is created with default value and the CRC is still not change?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on pull request #10468: Make column order deterministic in segment
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on PR #10468:
URL: https://github.com/apache/pinot/pull/10468#issuecomment-1489711678
> A high level question, will this handle the scenario that during schema evolution, a new column is created with default value and the CRC is still not change?
Piggy back another high level question: why adding a column needs to change the original segment :)
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #10468: Make column order deterministic in segment
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10468:
URL: https://github.com/apache/pinot/pull/10468#discussion_r1152726183
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java:
##########
@@ -650,22 +655,19 @@ public boolean equals(Object o) {
if (EqualityUtils.isSameReference(this, o)) {
return true;
}
-
if (EqualityUtils.isNullOrNotSameClass(this, o)) {
return false;
}
-
Schema that = (Schema) o;
-
- return EqualityUtils.isEqual(_schemaName, that._schemaName) && EqualityUtils
- .isEqualIgnoreOrder(_dimensionFieldSpecs, that._dimensionFieldSpecs) && EqualityUtils
- .isEqualIgnoreOrder(_metricFieldSpecs, that._metricFieldSpecs) && EqualityUtils
- .isEqual(_timeFieldSpec, that._timeFieldSpec) && EqualityUtils
- .isEqualIgnoreOrder(_dateTimeFieldSpecs, that._dateTimeFieldSpecs) && EqualityUtils
- .isEqualIgnoreOrder(_complexFieldSpecs, that._complexFieldSpecs) && EqualityUtils
- .isEqualMap(_fieldSpecMap, that._fieldSpecMap) && EqualityUtils
- .isEqual(_primaryKeyColumns, that._primaryKeyColumns) && EqualityUtils
- .isEqual(_hasJSONColumn, that._hasJSONColumn);
+ //@formatter:off
+ return EqualityUtils.isEqual(_schemaName, that._schemaName)
Review Comment:
why removed _fieldSpecMap and _hasJSONColumn?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #10468: Make column order deterministic in segment
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10468:
URL: https://github.com/apache/pinot/pull/10468#discussion_r1147160435
##########
pinot-core/src/test/java/org/apache/pinot/core/util/CrcUtilsTest.java:
##########
@@ -19,71 +19,43 @@
package org.apache.pinot.core.util;
import java.io.File;
+import java.net.URL;
import java.util.concurrent.TimeUnit;
import org.apache.commons.io.FileUtils;
-import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader;
import org.apache.pinot.segment.local.segment.creator.SegmentTestUtils;
-import org.apache.pinot.segment.local.segment.creator.impl.SegmentCreationDriverFactory;
+import org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl;
import org.apache.pinot.segment.local.utils.CrcUtils;
-import org.apache.pinot.segment.spi.IndexSegment;
-import org.apache.pinot.segment.spi.SegmentMetadata;
import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig;
import org.apache.pinot.segment.spi.creator.SegmentIndexCreationDriver;
-import org.apache.pinot.spi.utils.ReadMode;
import org.apache.pinot.util.TestUtils;
-import org.testng.Assert;
import org.testng.annotations.Test;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
-/**
- * Dec 4, 2014
- */
public class CrcUtilsTest {
-
+ private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "CrcUtilsTest");
private static final String AVRO_DATA = "data/test_data-mv.avro";
- private static final File INDEX_DIR = new File("/tmp/testingCrc");
+ private static final long EXPECTED_CRC = 4139425029L;
@Test
- public void test1()
+ public void testCrc()
Review Comment:
Does this test different column orders give same CRC?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10468: Make column order deterministic in segment
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10468:
URL: https://github.com/apache/pinot/pull/10468#discussion_r1147922459
##########
pinot-core/src/test/java/org/apache/pinot/core/util/CrcUtilsTest.java:
##########
@@ -19,71 +19,43 @@
package org.apache.pinot.core.util;
import java.io.File;
+import java.net.URL;
import java.util.concurrent.TimeUnit;
import org.apache.commons.io.FileUtils;
-import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader;
import org.apache.pinot.segment.local.segment.creator.SegmentTestUtils;
-import org.apache.pinot.segment.local.segment.creator.impl.SegmentCreationDriverFactory;
+import org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl;
import org.apache.pinot.segment.local.utils.CrcUtils;
-import org.apache.pinot.segment.spi.IndexSegment;
-import org.apache.pinot.segment.spi.SegmentMetadata;
import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig;
import org.apache.pinot.segment.spi.creator.SegmentIndexCreationDriver;
-import org.apache.pinot.spi.utils.ReadMode;
import org.apache.pinot.util.TestUtils;
-import org.testng.Assert;
import org.testng.annotations.Test;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
-/**
- * Dec 4, 2014
- */
public class CrcUtilsTest {
-
+ private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "CrcUtilsTest");
private static final String AVRO_DATA = "data/test_data-mv.avro";
- private static final File INDEX_DIR = new File("/tmp/testingCrc");
+ private static final long EXPECTED_CRC = 4139425029L;
@Test
- public void test1()
+ public void testCrc()
Review Comment:
No, different column order will give different CRC. This test is used to make sure the order is always the same
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10468: Make column order deterministic in segment
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10468:
URL: https://github.com/apache/pinot/pull/10468#issuecomment-1486377519
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10468](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d9b574c) into [master](https://codecov.io/gh/apache/pinot/commit/a7b2b6736134b8283ad78eb873509ea1346a0de6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a7b2b67) will **decrease** coverage by `37.99%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10468 +/- ##
=============================================
- Coverage 70.28% 32.30% -37.99%
+ Complexity 6113 251 -5862
=============================================
Files 2070 2070
Lines 111981 111960 -21
Branches 17056 17061 +5
=============================================
- Hits 78703 36165 -42538
- Misses 27736 72585 +44849
+ Partials 5542 3210 -2332
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.62% <0.00%> (+0.08%)` | :arrow_up: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.99% <0.00%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...inputformat/avro/AvroIngestionSchemaValidator.java](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtYXZyby1iYXNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wbHVnaW4vaW5wdXRmb3JtYXQvYXZyby9BdnJvSW5nZXN0aW9uU2NoZW1hVmFsaWRhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...l/realtime/converter/RealtimeSegmentConverter.java](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvUmVhbHRpbWVTZWdtZW50Q29udmVydGVyLmphdmE=) | `0.00% <0.00%> (-79.25%)` | :arrow_down: |
| [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | `0.00% <ø> (-79.97%)` | :arrow_down: |
| [...t/creator/impl/SegmentIndexCreationDriverImpl.java](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50SW5kZXhDcmVhdGlvbkRyaXZlckltcGwuamF2YQ==) | `0.00% <0.00%> (-80.11%)` | :arrow_down: |
| [...ndex/converter/SegmentV1V2ToV3FormatConverter.java](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbnZlcnRlci9TZWdtZW50VjFWMlRvVjNGb3JtYXRDb252ZXJ0ZXIuamF2YQ==) | `0.00% <0.00%> (-78.42%)` | :arrow_down: |
| [.../columnminmaxvalue/ColumnMinMaxValueGenerator.java](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9jb2x1bW5taW5tYXh2YWx1ZS9Db2x1bW5NaW5NYXhWYWx1ZUdlbmVyYXRvci5qYXZh) | `0.00% <0.00%> (-74.63%)` | :arrow_down: |
| [...loader/defaultcolumn/BaseDefaultColumnHandler.java](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9kZWZhdWx0Y29sdW1uL0Jhc2VEZWZhdWx0Q29sdW1uSGFuZGxlci5qYXZh) | `0.00% <0.00%> (-57.97%)` | :arrow_down: |
| [.../local/segment/store/SingleFileIndexDirectory.java](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3N0b3JlL1NpbmdsZUZpbGVJbmRleERpcmVjdG9yeS5qYXZh) | `0.00% <0.00%> (-90.73%)` | :arrow_down: |
| [...org/apache/pinot/segment/local/utils/CrcUtils.java](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9DcmNVdGlscy5qYXZh) | `0.00% <ø> (-97.73%)` | :arrow_down: |
| [...he/pinot/segment/local/utils/TableConfigUtils.java](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9UYWJsZUNvbmZpZ1V0aWxzLmphdmE=) | `0.00% <0.00%> (-68.49%)` | :arrow_down: |
| ... and [4 more](https://codecov.io/gh/apache/pinot/pull/10468?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
... and [1263 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10468/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10468: Make column order deterministic in segment
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10468:
URL: https://github.com/apache/pinot/pull/10468#discussion_r1153577066
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java:
##########
@@ -650,22 +655,19 @@ public boolean equals(Object o) {
if (EqualityUtils.isSameReference(this, o)) {
return true;
}
-
if (EqualityUtils.isNullOrNotSameClass(this, o)) {
return false;
}
-
Schema that = (Schema) o;
-
- return EqualityUtils.isEqual(_schemaName, that._schemaName) && EqualityUtils
- .isEqualIgnoreOrder(_dimensionFieldSpecs, that._dimensionFieldSpecs) && EqualityUtils
- .isEqualIgnoreOrder(_metricFieldSpecs, that._metricFieldSpecs) && EqualityUtils
- .isEqual(_timeFieldSpec, that._timeFieldSpec) && EqualityUtils
- .isEqualIgnoreOrder(_dateTimeFieldSpecs, that._dateTimeFieldSpecs) && EqualityUtils
- .isEqualIgnoreOrder(_complexFieldSpecs, that._complexFieldSpecs) && EqualityUtils
- .isEqualMap(_fieldSpecMap, that._fieldSpecMap) && EqualityUtils
- .isEqual(_primaryKeyColumns, that._primaryKeyColumns) && EqualityUtils
- .isEqual(_hasJSONColumn, that._hasJSONColumn);
+ //@formatter:off
+ return EqualityUtils.isEqual(_schemaName, that._schemaName)
Review Comment:
They are derived fields (similarly we don't include `_dimensionNames` etc.). Ideally we should define them as transient, but java ser/de still need to serialize these fields.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org