You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "jackluo923 (via GitHub)" <gi...@apache.org> on 2024/10/23 18:54:53 UTC
[PR] [WIP] Add initial implementation immtuable CLPForwardIndex as associated classes [pinot]
jackluo923 opened a new pull request, #14288:
URL: https://github.com/apache/pinot/pull/14288
(no comment)
--
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
Re: [PR] Add immtuable CLPForwardIndex creator and related classes [pinot]
Posted by "jackluo923 (via GitHub)" <gi...@apache.org>.
jackluo923 commented on code in PR #14288:
URL: https://github.com/apache/pinot/pull/14288#discussion_r1835030704
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/forward/CLPMutableForwardIndexV2.java:
##########
@@ -226,11 +238,9 @@ public void setString(int docId, String value) {
* encoded message by replacing them with empty arrays, as Pinot does not accept null values.
*
* @param clpEncodedMessage The {@link EncodedMessage} to append.
- * @throws IOException if an I/O error occurs during the appending process.
*/
- public void appendEncodedMessage(@NotNull EncodedMessage clpEncodedMessage)
- throws IOException {
- if (_isClpEncoded) {
+ public void appendEncodedMessage(@NotNull EncodedMessage clpEncodedMessage) {
+ if (_isClpEncoded || _forceEnableClpEncoding) {
Review Comment:
It is triggered when `setForceEnableClpEncoding(boolean cond)` is called with the `true` value. Since CLPMutableForwardIndexV2 is expected to replace CLPMutableForwardIndex (V1) in the OSS code, we need this option to always enable CLP Encoding regardless of dictionary size.
--
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
Re: [PR] Add immtuable CLPForwardIndex creator and related classes [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #14288:
URL: https://github.com/apache/pinot/pull/14288#discussion_r1861079826
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/CLPForwardIndexCreatorV2.java:
##########
@@ -164,18 +180,29 @@ public CLPForwardIndexCreatorV2(File baseIndexDir, ColumnStatistics columnStatis
*
* @param baseIndexDir The base directory where the forward index files will be stored.
* @param clpMutableForwardIndex The mutable forward index containing the raw data to be ingested.
- * @param chunkCompressionType The compression type to be used for encoding the forward index.
+ * @param chunkCompressionType The compression type used for encoding the forward index.
+ * @param forceRawEncoding If true, raw bytes encoding will be used, bypassing CLP encoding.
* @throws IOException If there is an error during initialization or while accessing the file system.
*/
public CLPForwardIndexCreatorV2(File baseIndexDir, CLPMutableForwardIndexV2 clpMutableForwardIndex,
- ChunkCompressionType chunkCompressionType)
+ ChunkCompressionType chunkCompressionType, boolean forceRawEncoding)
throws IOException {
_chunkCompressionType = chunkCompressionType;
// Pick up metadata from mutable forward index and use it to initialize the immutable forward index
_column = clpMutableForwardIndex.getColumnName();
_numDoc = clpMutableForwardIndex.getNumDoc();
- _isClpEncoded = clpMutableForwardIndex.isClpEncoded();
+
+ _intermediateFilesDir =
Review Comment:
will we cleanup the intermediateFilesDir ?
--
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
Re: [PR] Add immtuable CLPForwardIndex creator and related classes [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu merged PR #14288:
URL: https://github.com/apache/pinot/pull/14288
--
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
Re: [PR] Add immtuable CLPForwardIndex creator and related classes [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #14288:
URL: https://github.com/apache/pinot/pull/14288#discussion_r1835015463
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/forward/CLPMutableForwardIndexV2.java:
##########
@@ -226,11 +238,9 @@ public void setString(int docId, String value) {
* encoded message by replacing them with empty arrays, as Pinot does not accept null values.
*
* @param clpEncodedMessage The {@link EncodedMessage} to append.
- * @throws IOException if an I/O error occurs during the appending process.
*/
- public void appendEncodedMessage(@NotNull EncodedMessage clpEncodedMessage)
- throws IOException {
- if (_isClpEncoded) {
+ public void appendEncodedMessage(@NotNull EncodedMessage clpEncodedMessage) {
+ if (_isClpEncoded || _forceEnableClpEncoding) {
Review Comment:
what is _forceEnableClpEncoding? can you provide an example that trigger this condition
--
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
Re: [PR] Add immtuable CLPForwardIndex creator and related classes [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #14288:
URL: https://github.com/apache/pinot/pull/14288#discussion_r1842942533
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictionaryColStatistics.java:
##########
@@ -125,4 +125,13 @@ public CLPStats getCLPStats() {
throw new IllegalStateException(
"CLP stats not available for column: " + _dataSourceMetadata.getFieldSpec().getName());
}
+
Review Comment:
```
public CLPStats getCLPStats() {
if (_forwardIndex instanceof CLPMutableForwardIndex) {
return ((CLPMutableForwardIndex) _forwardIndex).getCLPStats();
} else if (_forwardIndex instanceof CLPMutableForwardIndexV2) {
return ((CLPMutableForwardIndexV2) _forwardIndex).getCLPStats();
}
```
Saw similar code in getCLPStats() function, can you please let me know in which scenarios these functions are being called.
--
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
Re: [PR] Add immtuable CLPForwardIndex creator and related classes [pinot]
Posted by "jackluo923 (via GitHub)" <gi...@apache.org>.
jackluo923 commented on code in PR #14288:
URL: https://github.com/apache/pinot/pull/14288#discussion_r1861223235
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/CLPForwardIndexCreatorV2.java:
##########
@@ -164,18 +180,29 @@ public CLPForwardIndexCreatorV2(File baseIndexDir, ColumnStatistics columnStatis
*
* @param baseIndexDir The base directory where the forward index files will be stored.
* @param clpMutableForwardIndex The mutable forward index containing the raw data to be ingested.
- * @param chunkCompressionType The compression type to be used for encoding the forward index.
+ * @param chunkCompressionType The compression type used for encoding the forward index.
+ * @param forceRawEncoding If true, raw bytes encoding will be used, bypassing CLP encoding.
* @throws IOException If there is an error during initialization or while accessing the file system.
*/
public CLPForwardIndexCreatorV2(File baseIndexDir, CLPMutableForwardIndexV2 clpMutableForwardIndex,
- ChunkCompressionType chunkCompressionType)
+ ChunkCompressionType chunkCompressionType, boolean forceRawEncoding)
throws IOException {
_chunkCompressionType = chunkCompressionType;
// Pick up metadata from mutable forward index and use it to initialize the immutable forward index
_column = clpMutableForwardIndex.getColumnName();
_numDoc = clpMutableForwardIndex.getNumDoc();
- _isClpEncoded = clpMutableForwardIndex.isClpEncoded();
+
+ _intermediateFilesDir =
Review Comment:
Yes, it is cleaned up in the `close()` method
```
@Override
public void close()
throws IOException {
// Delete all temp files
FileUtils.deleteDirectory(_intermediateFilesDir); <- delete directory
_dataFile.close();
}
```
--
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
Re: [PR] Add immtuable CLPForwardIndex creator and related classes [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #14288:
URL: https://github.com/apache/pinot/pull/14288#discussion_r1842942533
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictionaryColStatistics.java:
##########
@@ -125,4 +125,13 @@ public CLPStats getCLPStats() {
throw new IllegalStateException(
"CLP stats not available for column: " + _dataSourceMetadata.getFieldSpec().getName());
}
+
Review Comment:
```
public CLPStats getCLPStats() {
if (_forwardIndex instanceof CLPMutableForwardIndex) {
return ((CLPMutableForwardIndex) _forwardIndex).getCLPStats();
} else if (_forwardIndex instanceof CLPMutableForwardIndexV2) {
return ((CLPMutableForwardIndexV2) _forwardIndex).getCLPStats();
}
```
Saw similar code in getCLPStats() function, can you please educate me in which scenarios these functions are being called.
also should it be changed to
```
return ((CLPMutableForwardIndexV2) _forwardIndex).getCLPStats();
return ((CLPMutableForwardIndexV2) _forwardIndex).getCLPV2Stats();
```
--
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
Re: [PR] Add immtuable CLPForwardIndex creator and related classes [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #14288:
URL: https://github.com/apache/pinot/pull/14288#discussion_r1842942533
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictionaryColStatistics.java:
##########
@@ -125,4 +125,13 @@ public CLPStats getCLPStats() {
throw new IllegalStateException(
"CLP stats not available for column: " + _dataSourceMetadata.getFieldSpec().getName());
}
+
Review Comment:
```
public CLPStats getCLPStats() {
if (_forwardIndex instanceof CLPMutableForwardIndex) {
return ((CLPMutableForwardIndex) _forwardIndex).getCLPStats();
} else if (_forwardIndex instanceof CLPMutableForwardIndexV2) {
return ((CLPMutableForwardIndexV2) _forwardIndex).getCLPStats();
}
```
Saw similar code in getCLPStats() function, can you please educate me in which scenarios these functions are being called.
--
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
Re: [PR] [WIP] Add immtuable CLPForwardIndex and related classes [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14288:
URL: https://github.com/apache/pinot/pull/14288#issuecomment-2433284807
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14288?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`81b0435`)](https://app.codecov.io/gh/apache/pinot/commit/81b04354564adc4b2cc705657dd63f13bb0fd812?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1225 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (81b0435). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (81b0435) |
>|------|------|------|
>|integration|7|1|
>|integration2|3|1|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|0|
>|skip-bytebuffers-false|7|1|
>|unittests|5|0|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #14288 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 3 -2433
Lines 133233 6 -133227
Branches 20636 0 -20636
=============================================
- Hits 82274 0 -82274
+ Misses 44911 6 -44905
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.75%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/14288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/14288?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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
Re: [PR] Add immtuable CLPForwardIndex creator and related classes [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #14288:
URL: https://github.com/apache/pinot/pull/14288#discussion_r1835015463
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/forward/CLPMutableForwardIndexV2.java:
##########
@@ -226,11 +238,9 @@ public void setString(int docId, String value) {
* encoded message by replacing them with empty arrays, as Pinot does not accept null values.
*
* @param clpEncodedMessage The {@link EncodedMessage} to append.
- * @throws IOException if an I/O error occurs during the appending process.
*/
- public void appendEncodedMessage(@NotNull EncodedMessage clpEncodedMessage)
- throws IOException {
- if (_isClpEncoded) {
+ public void appendEncodedMessage(@NotNull EncodedMessage clpEncodedMessage) {
+ if (_isClpEncoded || _forceEnableClpEncoding) {
Review Comment:
what is _forceEnableClpEncoding? can you provide an example?
--
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
Re: [PR] Add immtuable CLPForwardIndex creator and related classes [pinot]
Posted by "jackluo923 (via GitHub)" <gi...@apache.org>.
jackluo923 commented on code in PR #14288:
URL: https://github.com/apache/pinot/pull/14288#discussion_r1844497055
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableNoDictionaryColStatistics.java:
##########
@@ -125,4 +125,13 @@ public CLPStats getCLPStats() {
throw new IllegalStateException(
"CLP stats not available for column: " + _dataSourceMetadata.getFieldSpec().getName());
}
+
Review Comment:
ClpMutableForwardIndexV2 index -> ClpForwardIndexCreator (V1): use `getCLPStats()`
ClpMutableForwardIndexV2 index -> ClpForwardIndexCreatorV2: use `getCLPV2Stats()`
`getCLPStats()` method is basically for compatibility with the initial version of immutable forward index.
--
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