You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "shounakmk219 (via GitHub)" <gi...@apache.org> on 2023/12/28 14:51:53 UTC
[PR] FileWriter bug fixes [pinot]
shounakmk219 opened a new pull request, #12208:
URL: https://github.com/apache/pinot/pull/12208
This PR includes 2 bug fixes
1. `FileWriter` was appending header line by default. Pushed this down to `CsvWriter` where its required.
2. Ensure that the number of rows written to the files is equal to the value provided in the spec.
--
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] FileWriter bug fixes [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12208:
URL: https://github.com/apache/pinot/pull/12208#discussion_r1439355717
##########
pinot-controller/src/main/java/org/apache/pinot/controller/recommender/data/writer/CsvWriter.java:
##########
@@ -38,6 +42,24 @@ protected String generateRow(DataGenerator generator) {
return StringUtils.join(values, ",");
}
+ @Override
+ public void write()
+ throws Exception {
+ super.write();
Review Comment:
That's right. Made the changes.
--
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] FileWriter bug fixes [pinot]
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on code in PR #12208:
URL: https://github.com/apache/pinot/pull/12208#discussion_r1439339946
##########
pinot-controller/src/main/java/org/apache/pinot/controller/recommender/data/writer/CsvWriter.java:
##########
@@ -38,6 +42,24 @@ protected String generateRow(DataGenerator generator) {
return StringUtils.join(values, ",");
}
+ @Override
+ public void write()
+ throws Exception {
+ super.write();
Review Comment:
IIUC, this go over the entire file twice? And repeats this for each file?
Could we avoid this by having the base FileWriter have a `init(FileWriter writer)` function that can be called in the original write loop each time a file is created, and the CSVWriter can override it to add the headers to avoid the 2nd pass?
--
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] FileWriter bug fixes [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12208:
URL: https://github.com/apache/pinot/pull/12208#issuecomment-1871273651
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12208?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:
> Comparison is base [(`50912eb`)](https://app.codecov.io/gh/apache/pinot/commit/50912eb0c419151bdaa23fa5153d93dfab4bf9c6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.52% compared to head [(`414b24d`)](https://app.codecov.io/gh/apache/pinot/pull/12208?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 46.41%.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12208 +/- ##
=============================================
- Coverage 61.52% 46.41% -15.11%
+ Complexity 1152 943 -209
=============================================
Files 2415 1809 -606
Lines 131157 95158 -35999
Branches 20245 15352 -4893
=============================================
- Hits 80692 44171 -36521
- Misses 44571 47833 +3262
+ Partials 5894 3154 -2740
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12208/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/12208/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/12208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12208/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/12208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12208/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/12208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.41% <ø> (-14.99%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.41% <ø> (-15.09%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12208/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/12208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.41% <ø> (-15.11%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.41% <ø> (-15.11%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.41% <ø> (-0.20%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12208/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/12208?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] FileWriter bug fixes [pinot]
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on code in PR #12208:
URL: https://github.com/apache/pinot/pull/12208#discussion_r1439359979
##########
pinot-controller/src/main/java/org/apache/pinot/controller/recommender/data/writer/CsvWriter.java:
##########
@@ -38,6 +38,13 @@ protected String generateRow(DataGenerator generator) {
return StringUtils.join(values, ",");
}
+ @Override
+ protected void preprocess(java.io.FileWriter writer)
+ throws Exception {
Review Comment:
Why does this function throw exceptions?
--
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] FileWriter bug fixes [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12208:
URL: https://github.com/apache/pinot/pull/12208#discussion_r1439364282
##########
pinot-controller/src/main/java/org/apache/pinot/controller/recommender/data/writer/CsvWriter.java:
##########
@@ -38,6 +38,13 @@ protected String generateRow(DataGenerator generator) {
return StringUtils.join(values, ",");
}
+ @Override
+ protected void preprocess(java.io.FileWriter writer)
+ throws Exception {
+ final String headers = StringUtils.join(_spec.getGenerator().nextRow().keySet(), ",");
Review Comment:
It won't be wrong, as we can generate as many rows as we want but yes it makes sense to move it to init() to avoid the rework for each file.
--
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] FileWriter bug fixes [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #12208:
URL: https://github.com/apache/pinot/pull/12208#discussion_r1439366935
##########
pinot-controller/src/main/java/org/apache/pinot/controller/recommender/data/writer/CsvWriter.java:
##########
@@ -38,6 +38,13 @@ protected String generateRow(DataGenerator generator) {
return StringUtils.join(values, ",");
}
+ @Override
+ protected void preprocess(java.io.FileWriter writer)
+ throws Exception {
Review Comment:
`preprocess(`) will work with the `FileWriter` object, here doing an `append()`, hence relaying the exception back so that it is handled by the caller.
--
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] FileWriter bug fixes [pinot]
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on code in PR #12208:
URL: https://github.com/apache/pinot/pull/12208#discussion_r1439359658
##########
pinot-controller/src/main/java/org/apache/pinot/controller/recommender/data/writer/CsvWriter.java:
##########
@@ -38,6 +38,13 @@ protected String generateRow(DataGenerator generator) {
return StringUtils.join(values, ",");
}
+ @Override
+ protected void preprocess(java.io.FileWriter writer)
+ throws Exception {
+ final String headers = StringUtils.join(_spec.getGenerator().nextRow().keySet(), ",");
Review Comment:
I think this will be wrong since we'll end up calling nextRow() for each file, instead we should do it just once. Maybe override init() in this class and store the headers?
--
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] FileWriter bug fixes [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #12208:
URL: https://github.com/apache/pinot/pull/12208
--
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