[Impala-ASF-CR] IMPALA-5341: Avoid unintended filter out in fe test

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8543 )

Change subject: IMPALA-5341: Avoid unintended filter out in fe test
..


Patch Set 1:

There is a false positive in PlannerTest#testKuduSelectivity. I've fixed a 
row-size value. I confirmed that the following check was finished without any 
error:

mvn -fae test -Dtest=PlannerTest

Is there any missing test?


--
To view, visit http://gerrit.cloudera.org:8080/8543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie79f644a37b0ffab7b0d8e94e77650d56423697a
Gerrit-Change-Number: 8543
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 15 Nov 2017 07:34:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5341: Avoid unintended filter out in fe test

[Kim Jin Chul (Code Review)]

Hello Jim Apple, Alex Behm,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8543

to look at the new patch set (#2).

Change subject: IMPALA-5341: Avoid unintended filter out in fe test
..

IMPALA-5341: Avoid unintended filter out in fe test

Change-Id: Ie79f644a37b0ffab7b0d8e94e77650d56423697a
---
M fe/src/test/java/org/apache/impala/testutil/TestUtils.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
2 files changed, 2 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/8543/2
--
To view, visit http://gerrit.cloudera.org:8080/8543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie79f644a37b0ffab7b0d8e94e77650d56423697a
Gerrit-Change-Number: 8543
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8489 )

Change subject: IMPALA-6084: Avoid using of global namespace for llvm
..


Patch Set 4:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-anyval.cc@156
PS4, Line 156: // DecimalVal,
> you can fit this in the previous line
Done


http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-callgraph.cc
File be/src/codegen/codegen-callgraph.cc:

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/codegen-callgraph.cc@41
PS4, Line 41: for (llvm::Use& u : val->uses())
> I think we usually stick to using no space between ":" like
Done for removal of the whitespace


http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/instruction-counter.cc
File be/src/codegen/instruction-counter.cc:

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/instruction-counter.cc@43
PS4, Line 43:   // Create all instruction counter and put them into counters_. 
Any
:   // InstructionCount that has instructions delegated to it in
:   // InstructionCounter::visit(const Instruction )
> can fit in 2 lines
Done


http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/codegen/llvm-codegen.cc@1127
PS4, Line 1127: llvm::
> search/replace gone wrong
Done


http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/hash-table.cc@40
PS4, Line 40: using llvm::APFloat;
: using llvm::ArrayRef;
: using llvm::BasicBlock;
: using llvm::ConstantFP;
: using llvm::Function;
: using llvm::LLVMContext;
: using llvm::PHINode;
: using llvm::PointerType;
: using llvm::Type;
: using llvm::Value;
> you can remove these
Done


http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-builder.cc
File be/src/exec/partitioned-hash-join-builder.cc:

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-builder.cc@49
PS4, Line 49: using llvm::ConstantInt;
: using llvm::Function;
: using llvm::LLVMContext;
: using llvm::PointerType;
: using llvm::Type;
: using llvm::Value
> you can remove these
Done


http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-node.cc
File be/src/exec/partitioned-hash-join-node.cc:

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-node.cc@50
PS4, Line 50: using llvm::BasicBlock;
: using llvm::ConstantInt;
: using llvm::Function;
: using llvm::GlobalValue;
: using llvm::LLVMContext;
: using llvm::PointerType;
: using llvm::Type;
: using llvm::Value;
> you can remove these
Done


http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/partitioned-hash-join-node.cc@131
PS4, Line 131: llvm::
> search/replace gone wrong
Done


http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/select-node.cc
File be/src/exec/select-node.cc:

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exec/select-node.cc@30
PS4, Line 30: using llvm::Function;
> you can remove this
Done


http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-expr.cc
File be/src/exprs/scalar-expr.cc:

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-expr.cc@260
PS4, Line 260: llvm::
> search/replace gone wrong, after you fix that, you can fit this into one li
Done


http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/exprs/scalar-fn-call.cc@46
PS4, Line 46: using llvm::ArrayType;
: using llvm::BasicBlock;
: using llvm::Function;
: using llvm::GlobalVariable;
: using llvm::PointerType;
: using llvm::Type;
: using llvm::Value;
> you can remove these
Done


http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:

http://gerrit.cloudera.org:8080/#/c/8489/4/be/src/runtime/descriptors.cc@40
PS4, Line 40: using llvm::Constant;
: using llvm::ConstantAggregateZero;
: using llvm::ConstantInt;
: using llvm::ConstantStruct;
: using llvm::StructType;
> you can remove these
Done



--
To view, visit http://gerrit.cloudera.org:8080/8489
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: 

[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm

[Kim Jin Chul (Code Review)]

Hello Philip Zeyliger, Tim Armstrong, Bikramjeet Vig,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8489

to look at the new patch set (#5).

Change subject: IMPALA-6084: Avoid using of global namespace for llvm
..

IMPALA-6084: Avoid using of global namespace for llvm

There are a large number of places in the backend where
we import everything from the llvm namespace into the
global namespace. (e.g. using namespace llvm;)

Here are the reasons why we don't prefer this:
* It could make symbol conflicts if a newly added code
has same symbole name.
* It makes code readability uncomfortable. We may not
recognize a symbol came from.

We adopt a sequence of namespace specifiers in each use case.

Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/codegen-util.cc
M be/src/codegen/instruction-counter-test.cc
M be/src/codegen/instruction-counter.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/select-node.cc
M be/src/exec/text-converter.cc
M be/src/exec/topn-node.cc
M be/src/exec/union-node.cc
M be/src/exprs/agg-fn-evaluator.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/compound-predicates.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/literal.cc
M be/src/exprs/null-literal.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/slot-ref.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/types.cc
M be/src/util/tuple-row-compare.cc
42 files changed, 1,419 insertions(+), 1,385 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/8489/5
--
To view, visit http://gerrit.cloudera.org:8080/8489
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a
Gerrit-Change-Number: 8489
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format

[Kim Jin Chul (Code Review)]

Hello Attila Jeges, Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8508

to look at the new patch set (#3).

Change subject: IMPALA-5237: Support a quoted string in date/time format
..

IMPALA-5237: Support a quoted string in date/time format

Impala does not represent a quoted string at date/time format.
For example, addtional quoted string between the patterns
(e.g. HH\'H\' => 11H). Hive supports this feature, so user wants
to get a same result from Impala. By the way, Impala returns
a different result as below.

* Hive
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08H 39M 24S

* Impala
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08'8' 39'4' 24'0'

The change affects the format pattern for
from_unixtime/from_timestamp/unix_timestamp.

In unix_timestamp, user can also specify a quoted string like this.

> select unix_timestamp('\'Epoch time: \'19700101',
> '\'Epoch time: \'MMdd');
0

By the way, the quoted strings between input and format should be
exactly same and internally string comparison is case-sensitive.

There is one intentional difference between Hive and Impala.
In Impala, the format string should have any date or time patten
as below. Throwing the error/warning is better if Impala cannot
optimize the expression. User must rewrite the query and don't pay
the function call.

* Hive
> select from_unixtime(0, '\'Hello world!\'');
Hello world!

* Impala
> select from_unixtime(0, '\'Hello world!\'');
Bad date/time conversion format: 'Hello world!'

Testing:
Add unit tests for working and nonworking cases

Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
3 files changed, 92 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8508/3
--
To view, visit http://gerrit.cloudera.org:8080/8508
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Jim Apple, Attila Jeges,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8355

to look at the new patch set (#12).

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
std::mt19937 in the C++11 standard library shows better randomness
than rand_r.

Testing:
Revise unit test in expr-test
Add E2E test to random.test

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
4 files changed, 61 insertions(+), 30 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/12
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8355 )

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 11:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8355/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8355/8//COMMIT_MSG@11
PS8, Line 11: library
> typo: library
Done


http://gerrit.cloudera.org:8080/#/c/8355/8/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/8355/8/be/src/exprs/expr-test.cc@4660
PS8, Line 4660: const double expected_result = 
ConvertValue(GetValue(rand.str(), TYPE_DOUBLE));
> nit: Please wrap lines at 90 characters.
Done


http://gerrit.cloudera.org:8080/#/c/8355/6/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8355/6/be/src/exprs/math-functions-ir.cc@179
PS6, Line 179: }
> I'm not confident that works nicely with boost::thread and pthreads. I emai
Okay, I support your suggestion.


http://gerrit.cloudera.org:8080/#/c/8355/8/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8355/8/be/src/exprs/math-functions-ir.cc@163
PS8, Line 163:   if (!seed_arg->is_null) seed = seed_arg->val;
 : }
 : mt1
> nit: one line. Also, please remove the space after !
Done



--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 11
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 14 Nov 2017 22:26:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Jim Apple, Attila Jeges,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8355

to look at the new patch set (#11).

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
std::mt19937 in the C++11 standard library shows better randomness
than rand_r.

Testing:
Revise unit test in expr-test
Add E2E test to random.test

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
4 files changed, 60 insertions(+), 30 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/11
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 11
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Jim Apple, Attila Jeges,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8355

to look at the new patch set (#10).

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
std::mt19937 in the C++11 standard libarary shows better randomness
than rand_r.

Testing:
Revise unit test in expr-test
Add E2E test to random.test

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
4 files changed, 60 insertions(+), 30 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/10
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 10
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Jim Apple, Attila Jeges,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8355

to look at the new patch set (#9).

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
std::mt19937 in the C++11 standard libarary shows better randomness
than rand_r.

Testing:
Revise unit test in expr-test
Add E2E test to random.test

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
4 files changed, 61 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/9
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 9
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Jim Apple,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8355

to look at the new patch set (#8).

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
std::mt19937 in the C++11 standard libarary shows better randomness
than rand_r.

Testing:
Revise unit test in expr-test
Add E2E test to random.test

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
4 files changed, 61 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/8
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 8
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Jim Apple,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8355

to look at the new patch set (#7).

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
std::mt19937 in the C++11 standard libarary shows better randomness
than rand_r.

Testing:
Revise unit test in expr-test
Add E2E test to random.test

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
4 files changed, 61 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/7
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8355 )

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8355/6/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8355/6/be/src/exprs/math-functions-ir.cc@179
PS6, Line 179:   static std::uniform_real_distribution 
distribution(min, max);
> For non-PODs like std::uniform_real_distribution, you probably want to avoi
You're right. I think thread_local is better than static to avoid the guard: 
https://godbolt.org/g/sxLsnc



--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 14 Nov 2017 14:36:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8355 )

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8355/3/be/src/util/rand-util.h
File be/src/util/rand-util.h:

http://gerrit.cloudera.org:8080/#/c/8355/3/be/src/util/rand-util.h@28
PS3, Line 28:
> Thanks for adding the e2e tests. I'd still recommend the file removals refe
Done



--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 14 Nov 2017 08:56:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Jim Apple,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8355

to look at the new patch set (#6).

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
std::mt19937 in the C++11 standard libarary shows better randomness
than rand_r.

Testing:
Revise unit test in expr-test
Add E2E test to random.test

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
4 files changed, 61 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/6
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5341: Avoid unintended filter out in fe test

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8543 )

Change subject: IMPALA-5341: Avoid unintended filter out in fe test
..


Patch Set 1:

(1 comment)

I would like to add a test which contains wrong row-size value and the test 
should show that an error always happen. Unfortunately, I haven't found any 
similar case from testdata. I thought JUnit test, but E2E test should be 
required in this case.

http://gerrit.cloudera.org:8080/#/c/8543/1/fe/src/test/java/org/apache/impala/testutil/TestUtils.java
File fe/src/test/java/org/apache/impala/testutil/TestUtils.java:

http://gerrit.cloudera.org:8080/#/c/8543/1/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@96
PS1, Line 96: private final static String FILTER_KEY = "\\ssize=";
I think this approach is very simple, but it's enough to avoid the unexpected 
filter out. Please comment with examples if it might make any side effect.



--
To view, visit http://gerrit.cloudera.org:8080/8543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie79f644a37b0ffab7b0d8e94e77650d56423697a
Gerrit-Change-Number: 8543
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 14 Nov 2017 08:29:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5341: Avoid unintended filter out in fe test

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8543


Change subject: IMPALA-5341: Avoid unintended filter out in fe test
..

IMPALA-5341: Avoid unintended filter out in fe test

Change-Id: Ie79f644a37b0ffab7b0d8e94e77650d56423697a
---
M fe/src/test/java/org/apache/impala/testutil/TestUtils.java
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/8543/1
--
To view, visit http://gerrit.cloudera.org:8080/8543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie79f644a37b0ffab7b0d8e94e77650d56423697a
Gerrit-Change-Number: 8543
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Jim Apple,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8355

to look at the new patch set (#5).

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
std::mt19937 in the C++11 standard libarary shows better randomness
than rand_r.

Testing:
rand-util-test is newly addded. It checks randomness,
deterministic and range.

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
M be/src/util/CMakeLists.txt
A be/src/util/rand-util-test.cc
A be/src/util/rand-util.cc
A be/src/util/rand-util.h
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
8 files changed, 215 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/5
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8508 )

Change subject: IMPALA-5237: Support a quoted string in date/time format
..


Patch Set 1:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/exprs/expr-test.cc@5724
PS1, Line 5724:   TestValue("unix_timestamp('1970|01|01 00|00|00', '|MM|dd 
HH|mm|ss')", TYPE_BIGINT,
> I think we also need to add tests for parsing with format strings including
Thanks for the information.  A quoted text does not work In the first patch set 
due to missing implementation. Done.


http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/exprs/expr-test.cc@5746
PS1, Line 5746:   TestError("from_unixtime(0, 'HHH\\'')");
> Can you add a test for an unmatched '\. Or if one of the above tests covers
5745 and 5746 are testing for unterminated quote


http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.h
File be/src/runtime/timestamp-parse-util.h:

http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.h@77
PS1, Line 77: /// The following features are not supported:
> Can you update this comment to include an example of the feature you added?
Done


http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:

http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc@160
PS1, Line 160: if ('\'' == *str) {
> nit: *str == '\'' to match the convention below
Done


http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc@162
PS1, Line 162:   do {
> Can you add a comment here explaining that we're matching a string literal,
Done


http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc@164
PS1, Line 164: // Meet end of string
> This comment isn't to informative. Maybe:
Done


http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc@165
PS1, Line 165: if (str == str_end) {
> Nit: 1 line:
Done


http://gerrit.cloudera.org:8080/#/c/8508/1/be/src/runtime/timestamp-parse-util.cc@168
PS1, Line 168:   } while ('\'' != *str);
> nit: *str != '\'' to match the convention below
Done



--
To view, visit http://gerrit.cloudera.org:8080/8508
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 09 Nov 2017 06:28:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format

[Kim Jin Chul (Code Review)]

Hello Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8508

to look at the new patch set (#2).

Change subject: IMPALA-5237: Support a quoted string in date/time format
..

IMPALA-5237: Support a quoted string in date/time format

Impala does not represent a quoted string at date/time format. For example,
addtional quoted string between the patterns(e.g. HH\'H\' => 11H)
Hive supports this feature, so user wants to get a same result
from Impala. By the way, Impala returns a different result as below.

* Hive
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08H 39M 24S

* Impala
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08'8' 39'4' 24'0'

The change affects the format pattern for 
from_unixtime/from_timestamp/unix_timestamp.

In unix_timestamp, user can also specify a quoted string like this.

> select unix_timestamp('\'Epoch time: \'19700101', '\'Epoch time: \'MMdd');
0

By the way, the quoted strings between input and format should be exactly same
and internally string comparison is case-sensitive.

There is one intentional difference between Hive and Impala. In Impala, the 
format
string should have any date or time patten as below. Throwing the error/warning 
is
better if Impala cannot optimize the expression. User must rewrite the query and
don't pay the function call.

* Hive
> select from_unixtime(0, '\'Hello world!\'');
Hello world!

* Impala
> select from_unixtime(0, '\'Hello world!\'');
Bad date/time conversion format: 'Hello world!'

Testing:
Add unit tests for working and nonworking cases

Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
3 files changed, 81 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8508/2
--
To view, visit http://gerrit.cloudera.org:8080/8508
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8489 )

Change subject: IMPALA-6084: Avoid using of global namespace for llvm
..


Patch Set 4:

(2 comments)

@Tim and Bikram, both of you wants rollback the changes for only be/src/codegen 
due to llvm's symbol change in the future. Do I proceed it? Or do you need 
further discussion?

http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:

http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/codegen-anyval.cc@479
PS3, Line 479: casts it b
> search-and-replace gone wrong
Fixed unintentional changes


http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/instruction-counter.cc
File be/src/codegen/instruction-counter.cc:

http://gerrit.cloudera.org:8080/#/c/8489/3/be/src/codegen/instruction-counter.cc@28
PS3, Line 28: const char* InstructionCounter::TOTAL_INSTS = "Total 
Instructions";
> This was probably not intentional?
Fixed unintentional changes



--
To view, visit http://gerrit.cloudera.org:8080/8489
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a
Gerrit-Change-Number: 8489
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 09 Nov 2017 02:15:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm

[Kim Jin Chul (Code Review)]

Hello Philip Zeyliger, Tim Armstrong, Bikramjeet Vig,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8489

to look at the new patch set (#4).

Change subject: IMPALA-6084: Avoid using of global namespace for llvm
..

IMPALA-6084: Avoid using of global namespace for llvm

There are a large number of places in the backend where
we import everything from the llvm namespace into the
global namespace. (e.g. using namespace llvm;)

Here are the reasons why we don't prefer this:
* It could make symbol conflicts if a newly added code
has same symbole name.
* It makes code readability uncomfortable. We may not
recognize a symbol came from.

We adopt a sequence of namespace specifiers in each use case.

Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/codegen-util.cc
M be/src/codegen/instruction-counter-test.cc
M be/src/codegen/instruction-counter.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/select-node.cc
M be/src/exec/text-converter.cc
M be/src/exec/topn-node.cc
M be/src/exec/union-node.cc
M be/src/exprs/agg-fn-evaluator.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/compound-predicates.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/literal.cc
M be/src/exprs/null-literal.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/slot-ref.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/types.cc
M be/src/util/tuple-row-compare.cc
42 files changed, 1,420 insertions(+), 1,346 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/8489/4
--
To view, visit http://gerrit.cloudera.org:8080/8489
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a
Gerrit-Change-Number: 8489
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5237: support custom string in date/time format

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8508


Change subject: IMPALA-5237: support custom string in date/time format
..

IMPALA-5237: support custom string in date/time format

Impala does not represent a custom string at date/time format. For example,
addtional custom string between the patterns(e.g. HH\'H\' => 11H)
Hive supports this feature, so user wants to get a same result
from Impala. By the way, Impala returns a different result as below.

* Hive
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08H 39M 24S

* Impala
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08'8' 39'4' 24'0'

The change affects the format pattern for from_unixtime/from_timestamp.

There is one intentional difference between Hive and Impala.
In Impala, the format string should have any date or time patten as below.

* Hive
> select from_unixtime(0, '\'Hello world!\'');
Hello world!

* Impala
> select from_unixtime(0, '\'Hello world!\'');
Bad date/time conversion format: 'Hello world!'

Testing:
Add unit tests for working and nonworking cases

Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-parse-util.cc
2 files changed, 28 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8508/1
--
To view, visit http://gerrit.cloudera.org:8080/8508
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8355 )

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 3:

(6 comments)

E2E test has not been ready.

http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG@11
PS3, Line 11: in C++11 libarary
> "in the C++11 standard library"
Done


http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG@12
PS3, Line 12: because it has much longer period than that of rand in C.
> It does show better randomness, and it does have a longer period, but it do
Removed


http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG@13
PS3, Line 13: (More details in http://www.pcg-random.org/)
> That's a site promoting a different generator. If you want to list a citati
Removed


http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG@17
PS3, Line 17: t1
> This is not a reproducable test case without knowing what t1 is. You can us
Removed the example


http://gerrit.cloudera.org:8080/#/c/8355/3//COMMIT_MSG@23
PS3, Line 23: 
: * After
: > select count(distinct(rand(1))), count(*) from t1
: +---+---+
: | count(distinct (rand(1))) | count(*)  |
: +---+---+
: | 34603008  | 103809024 |
: +---+---+
:
: You may expect maximum randomness(e.g. 103809024).
: Due to the issue IMPALA-6117, randomness could be
: "maximum randomess / n". "n" means the number of Impala
: execution engines. n is 3 in this example and each
: execution engine loads and processes data in parallel.
:
: This change introduces a new utility code for random because
: we have a plan to replace the legacy in IMPALA-4954 with
: the utility code.
> I'd say this whole section is overkill.
Removed


http://gerrit.cloudera.org:8080/#/c/8355/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/8355/3/be/src/exprs/expr-test.cc@4652
PS3, Line 4652:   {
> You can reduce this code size and make it more robust to future changes to
Done



--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 08 Nov 2017 07:14:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Jim Apple,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8355

to look at the new patch set (#4).

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
std::mt19937 in the C++11 standard libarary shows better randomness
than rand_r.

Testing:
rand-util-test is newly addded. It checks randomness,
deterministic and range.

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
M be/src/util/CMakeLists.txt
A be/src/util/rand-util-test.cc
A be/src/util/rand-util.cc
A be/src/util/rand-util.h
6 files changed, 185 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/4
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 

[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/8489 )

Change subject: IMPALA-6084: Avoid using of global namespace for llvm
..

IMPALA-6084: Avoid using of global namespace for llvm

There are a large number of places in the backend where
we import everything from the llvm namespace into the
global namespace. (e.g. using namespace llvm;)

Here are the reasons why we don't prefer this:
* It could make symbol conflicts if a newly added code
has same symbole name.
* It makes code readability uncomfortable. We may not
recognize a symbol came from.

We adopt a sequence of namespace specifiers in each use case.

Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/codegen-util.cc
M be/src/codegen/instruction-counter-test.cc
M be/src/codegen/instruction-counter.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/exec/blocking-join-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/select-node.cc
M be/src/exec/text-converter.cc
M be/src/exec/topn-node.cc
M be/src/exec/union-node.cc
M be/src/exprs/agg-fn-evaluator.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/compound-predicates.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/literal.cc
M be/src/exprs/null-literal.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/slot-ref.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/types.cc
M be/src/util/tuple-row-compare.cc
41 files changed, 1,452 insertions(+), 1,378 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/8489/3
--
To view, visit http://gerrit.cloudera.org:8080/8489
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a
Gerrit-Change-Number: 8489
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

Hello Greg Rahn, Jim Apple, Zach Amsden,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8311

to look at the new patch set (#6).

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..

IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC

Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.

* Add the following fields to EXTRACT and DATE_PART:

  WEEK
  DOW
  DOY
  SECOND/SECONDS
  MICROSECOND/MICROSECONDS
  NANOSECOND/NANOSECONDS

* Add the following field to TRUNC:

  SS

* Testing:
  Extend unit tests in expr-test

Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 211 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8311/6
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

Hello Greg Rahn, Zach Amsden,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8311

to look at the new patch set (#5).

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..

IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC

Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.

* Add the following fields to EXTRACT and DATE_PART:

  WEEK
  DOW
  DOY
  SECOND/SECONDS
  MICROSECOND/MICROSECONDS
  NANOSECOND/NANOSECONDS

* Add the following field to TRUNC:

  SS

* Testing:
  Extend unit tests in expr-test

Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 210 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8311/5
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8311 )

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/expr-test.cc@6037
PS4, Line 6037: TYPE_BIGINT, 28123);
> This is a compatibility breaking change; we need to document the issue.
Should we document incompatibility change on this change? or do we document it 
on a different change if your release date is not upcoming?


http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8311/4/be/src/exprs/udf-builtins-ir.cc@134
PS4, Line 134:   return time.fractional_seconds() / NANOS_PER_MICRO / 
MICROS_PER_MILLI + time.seconds() * MILLIS_PER_SEC;
> Too confusing - please use / (NANOS_PER_MICRO * MICROS_PER_MILLI)
Done



--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 31 Oct 2017 05:58:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8311 )

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@120
PS2, Line 120:   return time.fractional_seconds() + time.seconds() * 1000 * 
1000 * 1000;
> Use NANOS_PER_SEC
Done


http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@125
PS2, Line 125:   return ExtractNanosecond(time) / 1000;
> Instead of dividing the result of another function, these should be impleme
Done


http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@130
PS2, Line 130:   return ExtractMicrosecond(time) / 1000;
> same comment applies
Done



--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Mon, 30 Oct 2017 12:03:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

Hello Greg Rahn, Zach Amsden,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8311

to look at the new patch set (#4).

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..

IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC

Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.

* Add the following fields to EXTRACT and DATE_PART:

  WEEK
  DOW
  DOY
  SECOND/SECONDS
  MICROSECOND/MICROSECONDS
  NANOSECOND/NANOSECONDS

* Add the following field to TRUNC:

  SS

* Testing:
  Extend unit tests in expr-test

Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 210 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8311/4
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8311 )

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/expr-test.cc@5959
PS2, Line 5959: "cast(trunc(cast('2012-09-10 01:02:03' as timestamp), 
'DAY') as string)",
> I don't think the additional test cases add any value unless you add greate
Done


http://gerrit.cloudera.org:8080/#/c/8311/2/common/thrift/Exprs.thrift
File common/thrift/Exprs.thrift:

http://gerrit.cloudera.org:8080/#/c/8311/2/common/thrift/Exprs.thrift@92
PS2, Line 92:   MILLISECONDS,
> We shouldn't have redundant field definitions here; instead we should be le
I think we need them even though they look like redundant. See my comment at 
line 194: https://gerrit.cloudera.org/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc



--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Fri, 27 Oct 2017 09:43:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

Hello Greg Rahn, Zach Amsden,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8311

to look at the new patch set (#3).

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..

IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC

Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.

* Add the following fields to EXTRACT and DATE_PART:

  WEEK
  DOW
  DOY
  SECOND/SECONDS
  MICROSECOND/MICROSECONDS
  NANOSECOND/NANOSECONDS

* Add the following field to TRUNC:

  SS

* Testing:
  Extend unit tests in expr-test

Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 210 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8311/3
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8311 )

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@120
PS2, Line 120:   return time.fractional_seconds() + time.seconds() * 1000 * 
1000 * 1000;
The seconds field, including fractional parts, multiplied by 1 000 000 000; 
note that this includes full seconds

11:22:33.123456789
=> 33123456789


http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@154
PS2, Line 154: BigIntVal
Return type should be changed from INT to BIGINT because INT data type's range 
cannot cover NANOSECOND.


http://gerrit.cloudera.org:8080/#/c/8311/2/be/src/exprs/udf-builtins-ir.cc@194
PS2, Line 194: case TExtractField::NANOSECONDS:
MILLISECONDS, MICROSECONDS and NANOSECONDS look like useless, but 
ExtractFromExpr::EXTRACT_FIELDS should require them.
The available field type check in fe needs the aliases. The check code is in
src/main/java/org/apache/impala/analysis/ExtractFromExpr.java



--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Thu, 19 Oct 2017 12:18:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8311


Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..

IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC

Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.

* Add the following fields to EXTRACT and DATE_PART:

  WEEK
  DOW
  DOY
  SECOND/SECONDS
  MICROSECOND/MICROSECONDS
  NANOSECOND/NANOSECONDS

* Add the following field to TRUNC:

  SS

* Testing:
  Extend unit tests in expr-test

Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 221 insertions(+), 84 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8311/2
--
To view, visit http://gerrit.cloudera.org:8080/8311
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If23ea310ddcc5c1c806b2b1ac24d24a4bd6aa4d9
Gerrit-Change-Number: 8311
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-6073: Fail on misconfigured CLASSPATH.

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8327 )

Change subject: IMPALA-6073: Fail on misconfigured CLASSPATH.
..


Patch Set 1: Code-Review-1

I think this check seems not to be enough. Let me leave a detailed comment on 
the JIRA ticket.


--
To view, visit http://gerrit.cloudera.org:8080/8327
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb92fa5cad8bbc29b620bec5904e45ed7aeff13d
Gerrit-Change-Number: 8327
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Thu, 19 Oct 2017 04:13:54 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change.

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..


Patch Set 5: Code-Review+1

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7414/5/be/src/gutil/hash/hash.h
File be/src/gutil/hash/hash.h:

Line 101
Unnecessary include because of std::hash


Line 117
MSVC stuff is removed


Line 242
MSVC stuff is removed


Line 305
MSVC stuff is removed


Line 318
MSVC stuff is removed


-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Kim Jin Chul (Code Review)]

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7414

to look at the new patch set (#5).

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..

IMPALA-5116: Remove deprecated hash_* types in gutil

The following class templates are substituted from C++11 standard.
__gnu_cxx::hash_map => std::unordered_map
__gnu_cxx::hash_set => std::unordered_set
__gnu_cxx::hash => std::hash

Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
---
M be/src/gutil/hash/hash.cc
M be/src/gutil/hash/hash.h
M be/src/gutil/strings/join.h
M be/src/gutil/strings/serialize.cc
M be/src/gutil/strings/serialize.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
7 files changed, 37 insertions(+), 185 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/7414/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change.

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..


Patch Set 4:

(2 comments)

Sorry for the late. Let me answer Jim's comment soon.

http://gerrit.cloudera.org:8080/#/c/7414/3/be/src/gutil/hash/hash.h
File be/src/gutil/hash/hash.h:

PS3, Line 96: using std::hash;
: 
> What code requires this remain?
I answered in the PS#4


Line 189: 
> What code requires that this section remain?
I answered in the PS#4


-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded a new patch set (#4).

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..

IMPALA-5116: Remove deprecated hash_* types in gutil

The following class templates are substituted from C++11 standard.
__gnu_cxx::hash_map => std::unordered_map
__gnu_cxx::hash_set => std::unordered_set
__gnu_cxx::hash => std::hash

Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
---
M be/src/gutil/hash/hash.cc
M be/src/gutil/hash/hash.h
M be/src/gutil/strings/join.h
M be/src/gutil/strings/serialize.cc
M be/src/gutil/strings/serialize.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
7 files changed, 37 insertions(+), 154 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/7414/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5529: Add additional function signatures for TRUNC()

[Kim Jin Chul (Code Review)]

Hello Impala Public Jenkins, Matthew Jacobs,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7450

to look at the new patch set (#4).

Change subject: IMPALA-5529: Add additional function signatures for TRUNC()
..

IMPALA-5529: Add additional function signatures for TRUNC()

The following signatures to be added:
+--+--+-+---+
| return type  | signature| binary type | is persistent 
|
+--+--+-+---+
| DECIMAL(*,*) | trunc(DECIMAL(*,*))  | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), BIGINT)  | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), INT) | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), SMALLINT)| BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), TINYINT) | BUILTIN | true  
|
| BIGINT   | trunc(DOUBLE)| BUILTIN | true  
|
+--+--+-+---+

Tests:
* Adds tests for the new builtin trunc()/dtrunc()

Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf
---
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 36 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/7450/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7450
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-5529: Add additional function signatures for TRUNC()

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change.

Change subject: IMPALA-5529: Add additional function signatures for TRUNC()
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7450/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 2205: "Cannot resolve DECIMAL precision and scale from NULL 
type.");
I have some questions for implicit type casting.

I know that NULL is unknown, so NULL does not have a datatype. By the way, it 
seems implicit type conversion happens in the function calls. I guess NULL 
might be converted into DECIMAL type on the line 2205, but NULL might not be 
converted into DECIMAL type on the line 2204. I think there should be a 
precedence of datatypes. Would you let me know where I could find information 
for precedence rule in document/code?


-- 
To view, visit http://gerrit.cloudera.org:8080/7450
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5529: Add additional function signatures for TRUNC()

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change.

Change subject: IMPALA-5529: Add additional function signatures for TRUNC()
..


Patch Set 2:

(2 comments)

Thanks for all your reviews!

http://gerrit.cloudera.org:8080/#/c/7450/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

PS2, Line 2197: AnalyzesOk("select truncate(cast(1.123 as decimal(10,3)))");
  : AnalyzesOk("select truncate(cast(1.123 as decimal(10,3)), 
0)");
  : AnalyzesOk("select truncate(cast(1.123 as decimal(10,3)), 
2)");
  : AnalyzesOk("select truncate(cast(1.123 as decimal(10,3)), 
5)");
  : AnalyzesOk("select truncate(cast(1.123 as decimal(10,3)), 
-1)");
> can you wrap this in a loop to test these cases for each of truncate, dtrun
Done


Line 2240: testDecimalExpr("truncate(1.23)", 
ScalarType.createDecimalType(1, 0));
> Similarly, wrap these in a loop.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7450
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5529: Add additional function signatures for TRUNC()

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded a new patch set (#3).

Change subject: IMPALA-5529: Add additional function signatures for TRUNC()
..

IMPALA-5529: Add additional function signatures for TRUNC()

The following signatures to be added:
+--+--+-+---+
| return type  | signature| binary type | is persistent 
|
+--+--+-+---+
| DECIMAL(*,*) | trunc(DECIMAL(*,*))  | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), BIGINT)  | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), INT) | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), SMALLINT)| BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), TINYINT) | BUILTIN | true  
|
| BIGINT   | trunc(DOUBLE)| BUILTIN | true  
|
+--+--+-+---+

Tests:
* Adds tests for the new builtin trunc()/dtrunc()

Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf
---
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 36 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/7450/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7450
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-5529: Add additional function signatures for TRUNC()

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded a new patch set (#2).

Change subject: IMPALA-5529: Add additional function signatures for TRUNC()
..

IMPALA-5529: Add additional function signatures for TRUNC()

The following signatures to be added:
+--+--+-+---+
| return type  | signature| binary type | is persistent 
|
+--+--+-+---+
| DECIMAL(*,*) | trunc(DECIMAL(*,*))  | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), BIGINT)  | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), INT) | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), SMALLINT)| BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), TINYINT) | BUILTIN | true  
|
| BIGINT   | trunc(DOUBLE)| BUILTIN | true  
|
+--+--+-+---+

Tests:
* Adds tests for the new builtin trunc()
1) Runs properly on expected signature
2) Throws an exception on unexpected signature

Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf
---
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 11 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/7450/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7450
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change.

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7414/1/be/src/gutil/hash/hash.h
File be/src/gutil/hash/hash.h:

Line 278
> I looked at your comment. I did not respond yet because it is premature for
I found the specialized code from 218 to 328 is unused. My conclusion is the 
code can be removed because of the reason below. I believe there is no logical 
issue on my change.

1. hash functions are only used in hash_map/hash_set
(Checking with the command: git grep __gnu_cxx src)
2. hash_map/hash_set => unordered_map/unordered_set
The key type is only 'string'. std::hash is specialized in C++11.


-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change.

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7414/1/be/src/gutil/hash/hash.h
File be/src/gutil/hash/hash.h:

Line 222
> I think you might be right. Where was it used before your patch? In the has
Yes, it seems the specializations for hash are only used in the hash_* 
container.

e.g.
00082   template,
00083class _EqualKey = equal_to<_Key>, class _Alloc = allocator<_Tp> >
00084 class hash_map

By the way, I have not found any use case to create an object for hash_* 
container, so the code is now unused. Do you think we should leave it to use it 
later? Or it is better to remove the code to improve code coverage?


Line 278
> My suggestion is to dig into this library and understand the details of wha
I understand what you worry about. Let me look into the code more deeply. 

By the way, did you check my comment in the previous patch set? It would be 
nice if you take a look at the comment: 
https://gerrit.cloudera.org/#/c/7414/3/be/src/gutil/hash/hash.h@a251


-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5529: Add additional function signatures for TRUNC()

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change.

Change subject: IMPALA-5529: Add additional function signatures for TRUNC()
..


Patch Set 1:

(4 comments)

Hi,

Would you please review my code with comments? Thanks.
I would like to add some unit tests into 
java/org/apache/impala/analysis/AnalyzeExprsTest.java. Do you think this is a 
right place?
Please let me know the location if you have E2E test set.

Best regards,
Jinchul

http://gerrit.cloudera.org:8080/#/c/7450/1/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:

PS1, Line 278:   [['trunc'], 'BIGINT', ['BIGINT'], ''],
 :   [['trunc'], 'INT', ['INT'], ''],
 :   [['trunc'], 'SMALLINT', ['SMALLINT'], ''],
 :   [['trunc'], 'TINYINT', ['TINYINT'], ''],
1) I believe there is nothing to do. It just returns an input value.
I think we don't have to call backend function such as Truncate/Round, so I 
would like to do nothing here as coalesce's implementation. By the way, my 
implementation seems to be insufficient because backend throws "Function trunc 
is not implemented". Would you please let me know if there is any special 
handling?

2) What do you think about the specified types such as a series of INT type? I 
guess we need to support more premitive types to work the function properly.


PS1, Line 282: 'BIGINT', ['DOUBLE']
In the description of the JIRA, the reporter wrote that double precision of the 
argument should be double precision, but I am wondering it is expected behavior 
because of "DECIMAL(*, *) = TRUNC(DECIMAL(*,*) [, *INT])" which means return 
type should be same of the first argument type.

If the return type should be DOUBLE, I guess we need to add implicit conversion.


PS1, Line 284: 'DOUBLE', ['DOUBLE', 'INT']
1) I think "DOUBLE = TRUNC(DOUBLE, *INT)" is required. Do you agree on this?

2) The author before my patch intended implicit type conversion because there 
aren't SMALLINT, TINYINT and so on. I think implicit conversion has the 
disadvantages:
* unnecessary type conversion cost
* (maybe) forget the original type


http://gerrit.cloudera.org:8080/#/c/7450/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

Line 392:fnName_.getFunction().equalsIgnoreCase("trunc") ||
This is required to determine result type.


-- 
To view, visit http://gerrit.cloudera.org:8080/7450
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5529: Add additional function signatures for TRUNC()

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7450

Change subject: IMPALA-5529: Add additional function signatures for TRUNC()
..

IMPALA-5529: Add additional function signatures for TRUNC()

The following signatures to be added:
+--+--+-+---+
| return type  | signature| binary type | is persistent 
|
+--+--+-+---+
| BIGINT   | trunc(BIGINT)| BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*))  | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), BIGINT)  | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), INT) | BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), SMALLINT)| BUILTIN | true  
|
| DECIMAL(*,*) | trunc(DECIMAL(*,*), TINYINT) | BUILTIN | true  
|
| BIGINT   | trunc(DOUBLE)| BUILTIN | true  
|
| INT  | trunc(INT)   | BUILTIN | true  
|
| SMALLINT | trunc(SMALLINT)  | BUILTIN | true  
|
| TIMESTAMP| trunc(TIMESTAMP, STRING) | BUILTIN | true  
|
| TINYINT  | trunc(TINYINT)   | BUILTIN | true  
|
+--+--+-+---+

Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf
---
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
2 files changed, 11 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/7450/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7450
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change.

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7414/3/be/src/gutil/hash/hash.h
File be/src/gutil/hash/hash.h:

PS3, Line 218: 
As I told in the previous comment, _gnu_ctx is replaced into std.

https://gerrit.cloudera.org/#/c/7414/1/be/src/gutil/hash/hash.h@a278


Line 251
You may know std already defined pointer type as below.

   /// Partial specializations for pointer types.
   template
 struct hash<_Tp*> : public __hash_base
 {
   size_t
   operator()(_Tp* __p) const noexcept
   { return reinterpret_cast(__p); }
 };

I guess your hash function is more faster than std's implementation, but there 
are pros and cons. I found the interesting article: 
https://stackoverflow.com/questions/20953390/what-is-the-fastest-hash-function-for-pointers

I have the following issue on this. Please answer it.

How to implement a customized hash specialization for pointer type.
I removed your code because it cannot be redefined. I think we need to 
introduce a new template class which extends std::hash if we want to keep 
own hash.


-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded a new patch set (#3).

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..

IMPALA-5116: Remove deprecated hash_* types in gutil

The following class templates are substituted from C++11 standard.
__gnu_cxx::hash_map => std::unordered_map
__gnu_cxx::hash_set => std::unordered_set
__gnu_cxx::hash => std::hash

Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
---
M be/src/gutil/hash/hash.cc
M be/src/gutil/hash/hash.h
M be/src/gutil/strings/join.h
M be/src/gutil/strings/serialize.cc
M be/src/gutil/strings/serialize.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
7 files changed, 37 insertions(+), 121 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/7414/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change.

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7414/1/be/src/gutil/hash/hash.h
File be/src/gutil/hash/hash.h:

Line 222
> OK, so it looks like you've added it back in. Before we can decide if it sh
I guess the reason is that __gnu_cxx::hash<> was not used anywhere.  I didn't 
find any use case for this. Would you please double check?


Line 278
> So, do you need it to still exist, and if so, how do you make sure std::has
I got it. My idea is that some specializations in __gnu_cxx should be moved to 
std namespace. I can be sure this will not create a potential problem. Do you 
agree on this? Or any suggestion?


-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded a new patch set (#2).

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..

IMPALA-5116: Remove deprecated hash_* types in gutil

The following class templates are substituted from C++11 standard.
__gnu_cxx::hash_map => std::unordered_map
__gnu_cxx::hash_set => std::unordered_set
__gnu_cxx::hash => std::hash

Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
---
M be/src/gutil/hash/hash.cc
M be/src/gutil/hash/hash.h
M be/src/gutil/strings/join.h
M be/src/gutil/strings/serialize.cc
M be/src/gutil/strings/serialize.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
7 files changed, 35 insertions(+), 106 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/7414/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7414
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 

[Impala-ASF-CR] IMPALA-5116: Removal of uses for the deprecated functions in gutil

[Kim Jin Chul (Code Review)]

Kim Jin Chul has abandoned this change.

Change subject: IMPALA-5116: Removal of uses for the deprecated functions in 
gutil
..


Abandoned

-- 
To view, visit http://gerrit.cloudera.org:8080/7413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ia4bd8ae4cc4ca5aa72e181ce17bac9d1c91f9fb3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5116: Removal of uses for the deprecated functions in gutil

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7413

Change subject: IMPALA-5116: Removal of uses for the deprecated functions in 
gutil
..

IMPALA-5116: Removal of uses for the deprecated functions in gutil

The following functions are substituted for C++11 standard.
__gnu_cxx::hash_map => std::unordered_map
__gnu_cxx::hash_set => std::unordered_set
__gnu_cxx::hash => std::hash

Change-Id: Ia4bd8ae4cc4ca5aa72e181ce17bac9d1c91f9fb3
---
M be/src/gutil/hash/hash.cc
M be/src/gutil/hash/hash.h
M be/src/gutil/strings/join.h
M be/src/gutil/strings/serialize.cc
M be/src/gutil/strings/serialize.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
7 files changed, 35 insertions(+), 221 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/7413/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia4bd8ae4cc4ca5aa72e181ce17bac9d1c91f9fb3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 

[Impala-ASF-CR] test

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7412

Change subject: test
..

test

Change-Id: I8e3f1e28004e9d2a7526c83e00fcd8ecd2e0abc2
---
M README.md
1 file changed, 2 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/7412/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8e3f1e28004e9d2a7526c83e00fcd8ecd2e0abc2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul