You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by Jinho Kim <jh...@apache.org> on 2014/03/05 11:12:18 UTC
Review Request 18766: TAJO-657: Missing table stat in RCFile
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18766/
-----------------------------------------------------------
Review request for Tajo.
Bugs: TAJO-657
https://issues.apache.org/jira/browse/TAJO-657
Repository: tajo
Description
-------
RCFile should add stat for fine-grained progress indicator.(TAJO-589)
Diffs
-----
tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querytasks.jsp 744c0148a18cd01ece8be71041984b3ac293a75c
tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java 6c1923c1427bdacbae4c66ca6d87ac1cbb39a6b7
tajo-storage/src/main/java/org/apache/tajo/storage/rcfile/RCFile.java 90d7f48e9294e5e51cd7bceda6188c89baeb01d1
tajo-storage/src/test/java/org/apache/tajo/storage/TestStorages.java f4ce46bea980ce86fa9cf498ccaffc2ce5953c72
Diff: https://reviews.apache.org/r/18766/diff/
Testing
-------
Thanks,
Jinho Kim
Re: Review Request 18766: TAJO-657: Missing table stat in RCFile
Posted by Jinho Kim <jh...@apache.org>.
> On March 5, 2014, 3:35 p.m., Hyunsik Choi wrote:
> > tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java, line 171
> > <https://reviews.apache.org/r/18766/diff/2/?file=510822#file510822line171>
> >
> > Even though its computation cost may be trivial, this boolean expression will evaluated the number of rows x the number of columns times. So, in some cases, it may be costly.
> >
> > Instead of checking both boolean variables, we could set one pre-computed boolean variable. It would be more efficient.
>
> Hyunsik Choi wrote:
> Please ignore my above comment. JIT will optimize the variable evaluation. So, it would be not costly. Nevertheless, I would like to suggest to use a single variable for readability. It is hard for other guys to guess the relationship among csv file, shuffle, and stats.
I've added comments and task status log change to debug level
- Jinho
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18766/#review36229
-----------------------------------------------------------
On March 6, 2014, 4:15 a.m., Jinho Kim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18766/
> -----------------------------------------------------------
>
> (Updated March 6, 2014, 4:15 a.m.)
>
>
> Review request for Tajo.
>
>
> Bugs: TAJO-657
> https://issues.apache.org/jira/browse/TAJO-657
>
>
> Repository: tajo
>
>
> Description
> -------
>
> RCFile should add stat for fine-grained progress indicator.(TAJO-589)
> 'Actual Processed Bytes' is 'decompressed bytes + overhead'
>
>
> Diffs
> -----
>
> tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/querymaster/QueryMasterManagerService.java 70d40e7b593025accd8c64023f02d58ba63ae598
> tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querydetail.jsp 94e5f2ed915c089001c1bc48a0ce79158b79f3e7
> tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querytasks.jsp 744c0148a18cd01ece8be71041984b3ac293a75c
> tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java 6c1923c1427bdacbae4c66ca6d87ac1cbb39a6b7
> tajo-storage/src/main/java/org/apache/tajo/storage/rcfile/RCFile.java 90d7f48e9294e5e51cd7bceda6188c89baeb01d1
> tajo-storage/src/test/java/org/apache/tajo/storage/TestCompressionStorages.java bec1556e2f8301d485438b93666da3b9ba5b58d3
> tajo-storage/src/test/java/org/apache/tajo/storage/TestStorages.java f4ce46bea980ce86fa9cf498ccaffc2ce5953c72
>
> Diff: https://reviews.apache.org/r/18766/diff/
>
>
> Testing
> -------
>
> mvn clean install
> real cluster
>
>
> Thanks,
>
> Jinho Kim
>
>
Re: Review Request 18766: TAJO-657: Missing table stat in RCFile
Posted by Hyunsik Choi <hy...@apache.org>.
> On March 6, 2014, 12:35 a.m., Hyunsik Choi wrote:
> > tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java, line 171
> > <https://reviews.apache.org/r/18766/diff/2/?file=510822#file510822line171>
> >
> > Even though its computation cost may be trivial, this boolean expression will evaluated the number of rows x the number of columns times. So, in some cases, it may be costly.
> >
> > Instead of checking both boolean variables, we could set one pre-computed boolean variable. It would be more efficient.
Please ignore my above comment. JIT will optimize the variable evaluation. So, it would be not costly. Nevertheless, I would like to suggest to use a single variable for readability. It is hard for other guys to guess the relationship among csv file, shuffle, and stats.
- Hyunsik
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18766/#review36229
-----------------------------------------------------------
On March 5, 2014, 9:06 p.m., Jinho Kim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18766/
> -----------------------------------------------------------
>
> (Updated March 5, 2014, 9:06 p.m.)
>
>
> Review request for Tajo.
>
>
> Bugs: TAJO-657
> https://issues.apache.org/jira/browse/TAJO-657
>
>
> Repository: tajo
>
>
> Description
> -------
>
> RCFile should add stat for fine-grained progress indicator.(TAJO-589)
> 'Actual Processed Bytes' is 'decompressed bytes + overhead'
>
>
> Diffs
> -----
>
> tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querytasks.jsp 744c0148a18cd01ece8be71041984b3ac293a75c
> tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java 6c1923c1427bdacbae4c66ca6d87ac1cbb39a6b7
> tajo-storage/src/main/java/org/apache/tajo/storage/rcfile/RCFile.java 90d7f48e9294e5e51cd7bceda6188c89baeb01d1
> tajo-storage/src/test/java/org/apache/tajo/storage/TestCompressionStorages.java bec1556e2f8301d485438b93666da3b9ba5b58d3
> tajo-storage/src/test/java/org/apache/tajo/storage/TestStorages.java f4ce46bea980ce86fa9cf498ccaffc2ce5953c72
>
> Diff: https://reviews.apache.org/r/18766/diff/
>
>
> Testing
> -------
>
> mvn clean install
> real cluster
>
>
> Thanks,
>
> Jinho Kim
>
>
Re: Review Request 18766: TAJO-657: Missing table stat in RCFile
Posted by Hyunsik Choi <hy...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18766/#review36229
-----------------------------------------------------------
The patch looks pretty good for me. I leaved one comment.
tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java
<https://reviews.apache.org/r/18766/#comment67170>
Even though its computation cost may be trivial, this boolean expression will evaluated the number of rows x the number of columns times. So, in some cases, it may be costly.
Instead of checking both boolean variables, we could set one pre-computed boolean variable. It would be more efficient.
- Hyunsik Choi
On March 5, 2014, 9:06 p.m., Jinho Kim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18766/
> -----------------------------------------------------------
>
> (Updated March 5, 2014, 9:06 p.m.)
>
>
> Review request for Tajo.
>
>
> Bugs: TAJO-657
> https://issues.apache.org/jira/browse/TAJO-657
>
>
> Repository: tajo
>
>
> Description
> -------
>
> RCFile should add stat for fine-grained progress indicator.(TAJO-589)
> 'Actual Processed Bytes' is 'decompressed bytes + overhead'
>
>
> Diffs
> -----
>
> tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querytasks.jsp 744c0148a18cd01ece8be71041984b3ac293a75c
> tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java 6c1923c1427bdacbae4c66ca6d87ac1cbb39a6b7
> tajo-storage/src/main/java/org/apache/tajo/storage/rcfile/RCFile.java 90d7f48e9294e5e51cd7bceda6188c89baeb01d1
> tajo-storage/src/test/java/org/apache/tajo/storage/TestCompressionStorages.java bec1556e2f8301d485438b93666da3b9ba5b58d3
> tajo-storage/src/test/java/org/apache/tajo/storage/TestStorages.java f4ce46bea980ce86fa9cf498ccaffc2ce5953c72
>
> Diff: https://reviews.apache.org/r/18766/diff/
>
>
> Testing
> -------
>
> mvn clean install
> real cluster
>
>
> Thanks,
>
> Jinho Kim
>
>
Re: Review Request 18766: TAJO-657: Missing table stat in RCFile
Posted by Hyunsik Choi <hy...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18766/#review36336
-----------------------------------------------------------
Ship it!
+1
Thanks for nice work.
- Hyunsik Choi
On March 6, 2014, 1:15 p.m., Jinho Kim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18766/
> -----------------------------------------------------------
>
> (Updated March 6, 2014, 1:15 p.m.)
>
>
> Review request for Tajo.
>
>
> Bugs: TAJO-657
> https://issues.apache.org/jira/browse/TAJO-657
>
>
> Repository: tajo
>
>
> Description
> -------
>
> RCFile should add stat for fine-grained progress indicator.(TAJO-589)
> 'Actual Processed Bytes' is 'decompressed bytes + overhead'
>
>
> Diffs
> -----
>
> tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/querymaster/QueryMasterManagerService.java 70d40e7b593025accd8c64023f02d58ba63ae598
> tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querydetail.jsp 94e5f2ed915c089001c1bc48a0ce79158b79f3e7
> tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querytasks.jsp 744c0148a18cd01ece8be71041984b3ac293a75c
> tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java 6c1923c1427bdacbae4c66ca6d87ac1cbb39a6b7
> tajo-storage/src/main/java/org/apache/tajo/storage/rcfile/RCFile.java 90d7f48e9294e5e51cd7bceda6188c89baeb01d1
> tajo-storage/src/test/java/org/apache/tajo/storage/TestCompressionStorages.java bec1556e2f8301d485438b93666da3b9ba5b58d3
> tajo-storage/src/test/java/org/apache/tajo/storage/TestStorages.java f4ce46bea980ce86fa9cf498ccaffc2ce5953c72
>
> Diff: https://reviews.apache.org/r/18766/diff/
>
>
> Testing
> -------
>
> mvn clean install
> real cluster
>
>
> Thanks,
>
> Jinho Kim
>
>
Re: Review Request 18766: TAJO-657: Missing table stat in RCFile
Posted by Jinho Kim <jh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18766/
-----------------------------------------------------------
(Updated March 6, 2014, 4:15 a.m.)
Review request for Tajo.
Bugs: TAJO-657
https://issues.apache.org/jira/browse/TAJO-657
Repository: tajo
Description
-------
RCFile should add stat for fine-grained progress indicator.(TAJO-589)
'Actual Processed Bytes' is 'decompressed bytes + overhead'
Diffs (updated)
-----
tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/querymaster/QueryMasterManagerService.java 70d40e7b593025accd8c64023f02d58ba63ae598
tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querydetail.jsp 94e5f2ed915c089001c1bc48a0ce79158b79f3e7
tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querytasks.jsp 744c0148a18cd01ece8be71041984b3ac293a75c
tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java 6c1923c1427bdacbae4c66ca6d87ac1cbb39a6b7
tajo-storage/src/main/java/org/apache/tajo/storage/rcfile/RCFile.java 90d7f48e9294e5e51cd7bceda6188c89baeb01d1
tajo-storage/src/test/java/org/apache/tajo/storage/TestCompressionStorages.java bec1556e2f8301d485438b93666da3b9ba5b58d3
tajo-storage/src/test/java/org/apache/tajo/storage/TestStorages.java f4ce46bea980ce86fa9cf498ccaffc2ce5953c72
Diff: https://reviews.apache.org/r/18766/diff/
Testing
-------
mvn clean install
real cluster
Thanks,
Jinho Kim
Re: Review Request 18766: TAJO-657: Missing table stat in RCFile
Posted by Jinho Kim <jh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18766/
-----------------------------------------------------------
(Updated March 5, 2014, 12:06 p.m.)
Review request for Tajo.
Bugs: TAJO-657
https://issues.apache.org/jira/browse/TAJO-657
Repository: tajo
Description (updated)
-------
RCFile should add stat for fine-grained progress indicator.(TAJO-589)
'Actual Processed Bytes' is 'decompressed bytes + overhead'
Diffs
-----
tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querytasks.jsp 744c0148a18cd01ece8be71041984b3ac293a75c
tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java 6c1923c1427bdacbae4c66ca6d87ac1cbb39a6b7
tajo-storage/src/main/java/org/apache/tajo/storage/rcfile/RCFile.java 90d7f48e9294e5e51cd7bceda6188c89baeb01d1
tajo-storage/src/test/java/org/apache/tajo/storage/TestCompressionStorages.java bec1556e2f8301d485438b93666da3b9ba5b58d3
tajo-storage/src/test/java/org/apache/tajo/storage/TestStorages.java f4ce46bea980ce86fa9cf498ccaffc2ce5953c72
Diff: https://reviews.apache.org/r/18766/diff/
Testing
-------
mvn clean install
real cluster
Thanks,
Jinho Kim
Re: Review Request 18766: TAJO-657: Missing table stat in RCFile
Posted by Jinho Kim <jh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18766/
-----------------------------------------------------------
(Updated March 5, 2014, 12:02 p.m.)
Review request for Tajo.
Bugs: TAJO-657
https://issues.apache.org/jira/browse/TAJO-657
Repository: tajo
Description
-------
RCFile should add stat for fine-grained progress indicator.(TAJO-589)
Diffs
-----
tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querytasks.jsp 744c0148a18cd01ece8be71041984b3ac293a75c
tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java 6c1923c1427bdacbae4c66ca6d87ac1cbb39a6b7
tajo-storage/src/main/java/org/apache/tajo/storage/rcfile/RCFile.java 90d7f48e9294e5e51cd7bceda6188c89baeb01d1
tajo-storage/src/test/java/org/apache/tajo/storage/TestCompressionStorages.java bec1556e2f8301d485438b93666da3b9ba5b58d3
tajo-storage/src/test/java/org/apache/tajo/storage/TestStorages.java f4ce46bea980ce86fa9cf498ccaffc2ce5953c72
Diff: https://reviews.apache.org/r/18766/diff/
Testing (updated)
-------
mvn clean install
real cluster
Thanks,
Jinho Kim
Re: Review Request 18766: TAJO-657: Missing table stat in RCFile
Posted by Jinho Kim <jh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18766/
-----------------------------------------------------------
(Updated March 5, 2014, noon)
Review request for Tajo.
Bugs: TAJO-657
https://issues.apache.org/jira/browse/TAJO-657
Repository: tajo
Description
-------
RCFile should add stat for fine-grained progress indicator.(TAJO-589)
Diffs (updated)
-----
tajo-core/tajo-core-backend/src/main/resources/webapps/worker/querytasks.jsp 744c0148a18cd01ece8be71041984b3ac293a75c
tajo-storage/src/main/java/org/apache/tajo/storage/CSVFile.java 6c1923c1427bdacbae4c66ca6d87ac1cbb39a6b7
tajo-storage/src/main/java/org/apache/tajo/storage/rcfile/RCFile.java 90d7f48e9294e5e51cd7bceda6188c89baeb01d1
tajo-storage/src/test/java/org/apache/tajo/storage/TestCompressionStorages.java bec1556e2f8301d485438b93666da3b9ba5b58d3
tajo-storage/src/test/java/org/apache/tajo/storage/TestStorages.java f4ce46bea980ce86fa9cf498ccaffc2ce5953c72
Diff: https://reviews.apache.org/r/18766/diff/
Testing
-------
Thanks,
Jinho Kim