You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Sean Hsuan-Yi Chu <hs...@usc.edu> on 2015/01/07 01:53:35 UTC
Review Request 29648: DRILL-1874: Text reader should allow treating
empty fields as NULLs instead of empty string
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29648/
-----------------------------------------------------------
Review request for drill, Aman Sinha and Jinfeng Ni.
Bugs: DRILL-1874
https://issues.apache.org/jira/browse/DRILL-1874
Repository: drill-git
Description
-------
Users can choose to use safe (for varchar inputs)/default cast functions by setting system variable ("alter session set `planner.safe_cast_varchar` = true;").
Move the cast function replacement to "FunctionImplementationRegistry.java"
Add comments, Clean Codes
Diffs
-----
common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java 61d149d
exec/java-exec/src/main/codegen/data/Casts.tdd 36c90c9
exec/java-exec/src/main/codegen/templates/CastSafeFromNonnullableFunctions.java PRE-CREATION
exec/java-exec/src/main/codegen/templates/CastSafeFromNullableFunctions.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java b390cd5
exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java 0f40958
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java 055fdfa
exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 165d5f3
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 34729e2
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java e0cce8b
Diff: https://reviews.apache.org/r/29648/diff/
Testing
-------
unit tests,
functional tests,
tpch tests
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 29648: DRILL-1874: Text reader should allow
treating empty fields as NULLs instead of empty string
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29648/#review67029
-----------------------------------------------------------
A few comments:
- We should also support casting empty string to Decimal type since that is used often
- Why is the option defined in ExecConstants but is called planner.safe_cast_varchar ? Also, what is the rationale for choosing the prefix 'safe' ?
- Is it necessary to have separate template files for nullable and non-nullable cast if the only difference is in the Holder type for the input ?
- Are there any unit tests ? We will need to identify which cases the new Cast function gets applied because there's a dependency on Project pushdown such that the Project is above the Scan.
common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java
<https://reviews.apache.org/r/29648/#comment110856>
Can you import the DataMode class instead of the fully qualified reference here ?
- Aman Sinha
On Jan. 7, 2015, 2:12 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29648/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2015, 2:12 a.m.)
>
>
> Review request for drill, Aman Sinha and Jinfeng Ni.
>
>
> Bugs: DRILL-1874
> https://issues.apache.org/jira/browse/DRILL-1874
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Users can choose to use safe (for varchar inputs)/default cast functions by setting system variable ("alter session set `planner.safe_cast_varchar` = true;").
>
> Move the cast function replacement to "FunctionImplementationRegistry.java"
>
> Add comments, Clean Codes
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java 61d149d
> exec/java-exec/src/main/codegen/data/Casts.tdd 36c90c9
> exec/java-exec/src/main/codegen/templates/SafeCastFromNonnullableFunctions.java PRE-CREATION
> exec/java-exec/src/main/codegen/templates/SafeCastFromNullableFunctions.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java b390cd5
> exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java 0f40958
> exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java 055fdfa
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 165d5f3
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 34729e2
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java e0cce8b
>
> Diff: https://reviews.apache.org/r/29648/diff/
>
>
> Testing
> -------
>
> unit tests,
> functional tests,
> tpch tests
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 29648: DRILL-1874: Text reader should allow
treating empty fields as NULLs instead of empty string
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29648/#review67284
-----------------------------------------------------------
common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java
<https://reviews.apache.org/r/29648/#comment111281>
Change the name of the option...
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
<https://reviews.apache.org/r/29648/#comment111282>
shouldn't this be cast_empty_string_to_null instead of cast. ?
- Aman Sinha
On Jan. 8, 2015, 6:25 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29648/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2015, 6:25 p.m.)
>
>
> Review request for drill, Aman Sinha and Jinfeng Ni.
>
>
> Bugs: DRILL-1874
> https://issues.apache.org/jira/browse/DRILL-1874
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Users can choose to use safe (for varchar inputs)/default cast functions by setting system variable ("alter session set `planner.safe_cast_varchar` = true;").
>
> Move the cast function replacement to "FunctionImplementationRegistry.java"
>
> Add comments, Clean Codes
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java 61d149d
> exec/java-exec/src/main/codegen/data/Casts.tdd 36c90c9
> exec/java-exec/src/main/codegen/templates/CastVarCharToNullableNumeric.java PRE-CREATION
> exec/java-exec/src/main/codegen/templates/Decimal/CastVarCharToNullableDecimal.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java b390cd5
> exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java 0f40958
> exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java 055fdfa
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 165d5f3
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 34729e2
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java e0cce8b
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 30be58a
> exec/java-exec/src/test/resources/emptyStrings.csv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29648/diff/
>
>
> Testing
> -------
>
> unit tests,
> functional tests,
> tpch tests
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 29648: DRILL-1874: Text reader should allow
treating empty fields as NULLs instead of empty string
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29648/#review67349
-----------------------------------------------------------
exec/java-exec/src/main/codegen/data/Casts.tdd
<https://reviews.apache.org/r/29648/#comment111352>
Why do we need to use 'Safe' keyword in all the names here ? It's not necessarily 'safe' to do this casting (the default option value is false).
exec/java-exec/src/main/codegen/templates/Decimal/CastVarCharDecimal.java
<https://reviews.apache.org/r/29648/#comment111357>
Good that you were able to consolidate the new cast function with the existing decimal cast functions.
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
<https://reviews.apache.org/r/29648/#comment111354>
I am not completely sure if exec.functions is the right namespace for this option...you might want to get additional feedback.
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java
<https://reviews.apache.org/r/29648/#comment111347>
Could you create a separate test class TestCastEmptyString for this set of tests since there are quite a few that they deserve their own class. You could separate the nullable and non-nullable inputs into two separate tests.
- Aman Sinha
On Jan. 9, 2015, 12:14 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29648/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2015, 12:14 a.m.)
>
>
> Review request for drill, Aman Sinha and Jinfeng Ni.
>
>
> Bugs: DRILL-1874
> https://issues.apache.org/jira/browse/DRILL-1874
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Users can choose to use safe (for varchar inputs)/default cast functions by setting system variable ("alter session set `planner.safe_cast_varchar` = true;").
>
> Move the cast function replacement to "FunctionImplementationRegistry.java"
>
> Add comments, Clean Codes
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java 61d149d
> exec/java-exec/src/main/codegen/data/Casts.tdd 36c90c9
> exec/java-exec/src/main/codegen/templates/CastVarCharToNullableNumeric.java PRE-CREATION
> exec/java-exec/src/main/codegen/templates/Decimal/CastVarCharDecimal.java acf2f1d
> exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java b390cd5
> exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java 0f40958
> exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java 055fdfa
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 165d5f3
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 34729e2
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java e0cce8b
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 30be58a
> exec/java-exec/src/test/resources/emptyStrings.csv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29648/diff/
>
>
> Testing
> -------
>
> unit tests,
> functional tests,
> tpch tests
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 29648: DRILL-1874: Text reader should allow
treating empty fields as NULLs instead of empty string
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29648/#review67569
-----------------------------------------------------------
Ship it!
Looks like you've made the suggested changes in your github branch, so I am marking this as ship-it from my side (not sure if other reviewers have any more comments).
- Aman Sinha
On Jan. 9, 2015, 12:14 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29648/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2015, 12:14 a.m.)
>
>
> Review request for drill, Aman Sinha and Jinfeng Ni.
>
>
> Bugs: DRILL-1874
> https://issues.apache.org/jira/browse/DRILL-1874
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Users can choose to use safe (for varchar inputs)/default cast functions by setting system variable ("alter session set `planner.safe_cast_varchar` = true;").
>
> Move the cast function replacement to "FunctionImplementationRegistry.java"
>
> Add comments, Clean Codes
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java 61d149d
> exec/java-exec/src/main/codegen/data/Casts.tdd 36c90c9
> exec/java-exec/src/main/codegen/templates/CastVarCharToNullableNumeric.java PRE-CREATION
> exec/java-exec/src/main/codegen/templates/Decimal/CastVarCharDecimal.java acf2f1d
> exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java b390cd5
> exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java 0f40958
> exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java 055fdfa
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 165d5f3
> exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 34729e2
> exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java e0cce8b
> exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 30be58a
> exec/java-exec/src/test/resources/emptyStrings.csv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29648/diff/
>
>
> Testing
> -------
>
> unit tests,
> functional tests,
> tpch tests
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 29648: DRILL-1874: Text reader should allow
treating empty fields as NULLs instead of empty string
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29648/
-----------------------------------------------------------
(Updated Jan. 9, 2015, 12:14 a.m.)
Review request for drill, Aman Sinha and Jinfeng Ni.
Bugs: DRILL-1874
https://issues.apache.org/jira/browse/DRILL-1874
Repository: drill-git
Description
-------
Users can choose to use safe (for varchar inputs)/default cast functions by setting system variable ("alter session set `planner.safe_cast_varchar` = true;").
Move the cast function replacement to "FunctionImplementationRegistry.java"
Add comments, Clean Codes
Diffs (updated)
-----
common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java 61d149d
exec/java-exec/src/main/codegen/data/Casts.tdd 36c90c9
exec/java-exec/src/main/codegen/templates/CastVarCharToNullableNumeric.java PRE-CREATION
exec/java-exec/src/main/codegen/templates/Decimal/CastVarCharDecimal.java acf2f1d
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java b390cd5
exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java 0f40958
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java 055fdfa
exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 165d5f3
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 34729e2
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java e0cce8b
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 30be58a
exec/java-exec/src/test/resources/emptyStrings.csv PRE-CREATION
Diff: https://reviews.apache.org/r/29648/diff/
Testing
-------
unit tests,
functional tests,
tpch tests
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 29648: DRILL-1874: Text reader should allow
treating empty fields as NULLs instead of empty string
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29648/
-----------------------------------------------------------
(Updated Jan. 8, 2015, 6:25 p.m.)
Review request for drill, Aman Sinha and Jinfeng Ni.
Bugs: DRILL-1874
https://issues.apache.org/jira/browse/DRILL-1874
Repository: drill-git
Description
-------
Users can choose to use safe (for varchar inputs)/default cast functions by setting system variable ("alter session set `planner.safe_cast_varchar` = true;").
Move the cast function replacement to "FunctionImplementationRegistry.java"
Add comments, Clean Codes
Diffs (updated)
-----
common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java 61d149d
exec/java-exec/src/main/codegen/data/Casts.tdd 36c90c9
exec/java-exec/src/main/codegen/templates/CastVarCharToNullableNumeric.java PRE-CREATION
exec/java-exec/src/main/codegen/templates/Decimal/CastVarCharToNullableDecimal.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java b390cd5
exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java 0f40958
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java 055fdfa
exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 165d5f3
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 34729e2
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java e0cce8b
exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 30be58a
exec/java-exec/src/test/resources/emptyStrings.csv PRE-CREATION
Diff: https://reviews.apache.org/r/29648/diff/
Testing
-------
unit tests,
functional tests,
tpch tests
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 29648: DRILL-1874: Text reader should allow
treating empty fields as NULLs instead of empty string
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29648/
-----------------------------------------------------------
(Updated Jan. 7, 2015, 2:12 a.m.)
Review request for drill, Aman Sinha and Jinfeng Ni.
Bugs: DRILL-1874
https://issues.apache.org/jira/browse/DRILL-1874
Repository: drill-git
Description
-------
Users can choose to use safe (for varchar inputs)/default cast functions by setting system variable ("alter session set `planner.safe_cast_varchar` = true;").
Move the cast function replacement to "FunctionImplementationRegistry.java"
Add comments, Clean Codes
Diffs (updated)
-----
common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java 61d149d
exec/java-exec/src/main/codegen/data/Casts.tdd 36c90c9
exec/java-exec/src/main/codegen/templates/SafeCastFromNonnullableFunctions.java PRE-CREATION
exec/java-exec/src/main/codegen/templates/SafeCastFromNullableFunctions.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java b390cd5
exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java 0f40958
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java 055fdfa
exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 165d5f3
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java 34729e2
exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java e0cce8b
Diff: https://reviews.apache.org/r/29648/diff/
Testing
-------
unit tests,
functional tests,
tpch tests
Thanks,
Sean Hsuan-Yi Chu