[Impala-ASF-CR] IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21083 )

Change subject: IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax
..


Patch Set 4:

Build Started https://jenkins.impala.io/job/gerrit-docs-auto-test/755/

Testing docs change - this change appears to modify docs/ and no code. This is 
experimental - please report any issues to tarmstr...@cloudera.com or on this 
JIRA: IMPALA-7317


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb348d8395a6140f0be200d73e2f22fded9a5116
Gerrit-Change-Number: 21083
Gerrit-PatchSet: 4
Gerrit-Owner: Noemi Pap-Takacs 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Noemi Pap-Takacs 
Gerrit-Comment-Date: Mon, 11 Mar 2024 08:58:55 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax

2024-03-11 Thread Noemi Pap-Takacs (Code Review)
Hello Andrew Sherman, Daniel Becker, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax
..

IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax

Extended the ALTER TABLE documentation with the SORT BY clause.
Also added more information about the available and the deafult
sort orders to the CREATE TABLE description.

Testing: Built docs locally.

Change-Id: Ieb348d8395a6140f0be200d73e2f22fded9a5116
---
M docs/topics/impala_alter_table.xml
M docs/topics/impala_create_table.xml
2 files changed, 35 insertions(+), 7 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieb348d8395a6140f0be200d73e2f22fded9a5116
Gerrit-Change-Number: 21083
Gerrit-PatchSet: 4
Gerrit-Owner: Noemi Pap-Takacs 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Noemi Pap-Takacs 


[Impala-ASF-CR] IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21083 )

Change subject: IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax
..


Patch Set 4: Verified+1

Build Successful

https://jenkins.impala.io/job/gerrit-docs-auto-test/755/ : Doc tests passed.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb348d8395a6140f0be200d73e2f22fded9a5116
Gerrit-Change-Number: 21083
Gerrit-PatchSet: 4
Gerrit-Owner: Noemi Pap-Takacs 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Noemi Pap-Takacs 
Gerrit-Comment-Date: Mon, 11 Mar 2024 09:06:15 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11499: Refactor UrlEncode function to handle special characters

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21131 )

Change subject: IMPALA-11499: Refactor UrlEncode function to handle special 
characters
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21131/1/be/src/util/coding-util.cc
File be/src/util/coding-util.cc:

http://gerrit.cloudera.org:8080/#/c/21131/1/be/src/util/coding-util.cc@75
PS1, Line 75: static inline void UrlEncode(const char* in, int in_len, 
std::string* out, bool hive_compat) {
line too long (94 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I88c4aba5d811dfcec809583d0c16fcbc0ca730fb
Gerrit-Change-Number: 21131
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Comment-Date: Mon, 11 Mar 2024 09:58:52 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax

2024-03-11 Thread Noemi Pap-Takacs (Code Review)
Noemi Pap-Takacs has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21083 )

Change subject: IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax
..


Patch Set 4:

(1 comment)

Thank you for the review!

I think that both LEXICAL and ZORDER are common knowledge. I found a blog post 
about ZORDER in Impala and the description aligns with what one can find on 
Wikipedia.

http://gerrit.cloudera.org:8080/#/c/21083/3/docs/topics/impala_create_table.xml
File docs/topics/impala_create_table.xml:

http://gerrit.cloudera.org:8080/#/c/21083/3/docs/topics/impala_create_table.xml@456
PS3, Line 456: >
> Nit: comma before "which".
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb348d8395a6140f0be200d73e2f22fded9a5116
Gerrit-Change-Number: 21083
Gerrit-PatchSet: 4
Gerrit-Owner: Noemi Pap-Takacs 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Noemi Pap-Takacs 
Gerrit-Comment-Date: Mon, 11 Mar 2024 10:00:47 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax

2024-03-11 Thread Daniel Becker (Code Review)
Daniel Becker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21083 )

Change subject: IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb348d8395a6140f0be200d73e2f22fded9a5116
Gerrit-Change-Number: 21083
Gerrit-PatchSet: 4
Gerrit-Owner: Noemi Pap-Takacs 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Noemi Pap-Takacs 
Gerrit-Comment-Date: Mon, 11 Mar 2024 10:10:41 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax

2024-03-11 Thread Daniel Becker (Code Review)
Daniel Becker has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/21083 )

Change subject: IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax
..

IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax

Extended the ALTER TABLE documentation with the SORT BY clause.
Also added more information about the available and the deafult
sort orders to the CREATE TABLE description.

Testing: Built docs locally.

Change-Id: Ieb348d8395a6140f0be200d73e2f22fded9a5116
Reviewed-on: http://gerrit.cloudera.org:8080/21083
Tested-by: Impala Public Jenkins 
Reviewed-by: Daniel Becker 
---
M docs/topics/impala_alter_table.xml
M docs/topics/impala_create_table.xml
2 files changed, 35 insertions(+), 7 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Daniel Becker: Looks good to me, approved

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ieb348d8395a6140f0be200d73e2f22fded9a5116
Gerrit-Change-Number: 21083
Gerrit-PatchSet: 5
Gerrit-Owner: Noemi Pap-Takacs 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Noemi Pap-Takacs 


[Impala-ASF-CR] IMPALA-11499: Refactor UrlEncode function to handle special characters

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21131 )

Change subject: IMPALA-11499: Refactor UrlEncode function to handle special 
characters
..


Patch Set 1:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15474/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I88c4aba5d811dfcec809583d0c16fcbc0ca730fb
Gerrit-Change-Number: 21131
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Comment-Date: Mon, 11 Mar 2024 10:26:51 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12487: Skip reloading file metadata for ALTER TABLE events with trivial changes in StorageDescriptor

2024-03-11 Thread Quanlong Huang (Code Review)
Quanlong Huang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21019 )

Change subject: IMPALA-12487: Skip reloading file metadata for ALTER_TABLE 
events with trivial changes in StorageDescriptor
..


Patch Set 3:

(1 comment)

> Patch Set 2:
>
> > Patch Set 2:
> >
> > (2 comments)
> >
> > Skip reloading file metadata looks good to me. But I'm uncertain whether 
> > skip reloading table schema is safe. Maybe we should compare the whole 
> > msTable object instead of just checking several fields.
>
> Comparing the whole msTable object is not ideal because certain fields like 
> change columns, change owner, may require required table metadata reload but 
> certain other properties in serde like change location, and change row/file 
> format don't need table metadata reload. This is much more evident when we 
> use this patch in conjunction with IMPALA-10976.

I found a bug that changing the fileformat to AVRO does require table schema 
reload. Details in the comments.
I think the main purpose for this JIRA is to skip reloading file metadata which 
is expensive. Skip reloading the table schema is a minor optimization and 
brings risks. I'm not sure if there are other bugs like IMPALA-12889. So keeps 
reloading the table schema seems safer to me.
When testing IMPALA-10976, did you find any test failure if we don't skip 
reloading table schema?

http://gerrit.cloudera.org:8080/#/c/21019/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:

http://gerrit.cloudera.org:8080/#/c/21019/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1788
PS1, Line 1788: !Objects.equals(beforeSd.getSerdeInfo(), 
afterSd.getSerdeInfo())) {
> This behavior is consistent with the CatalogOpExecutor (i.e., command execu
Thanks for pointing to the existing codes. I found it's an existing issue that 
avro schema is not updated correctly when changing file format to AVRO. Filed 
IMPALA-12889.

The following scenario works before this patch:
1. Create a non-avro table with 'avro.schema.url' set to a valid schema file.
impala> create table my_part_tbl(i int) partitioned by (p int) stored as 
parquet;
impala> alter table my_part_tbl set 
tblproperties('avro.schema.url'='hdfs:test-warehouse/avro_schemas/functional/alltypes.json');

2. In HIVE, change the file format to AVRO
hive> alter table my_part_tbl set fileformat avro;

3. After Impala applies the ALTER_TABLE event, the schema should be the same:
hive> describe my_part_tbl;

 +--++--+
 | col_name | data_type  | comment  |
 +--++--+
 | id   | int|  |
 | bool_col | boolean|  |
 | tinyint_col  | int|  |
 | smallint_col | int|  |
 | int_col  | int|  |   
 | bigint_col   | bigint |  |
 | float_col| float  |  |
 | double_col   | double |  |
 | date_string_col  | string |  |
 | string_col   | string |  |
 | timestamp_col| string |  |
 | p| int|  |
 |  | NULL   | NULL |
 | # Partition Information  | NULL   | NULL |
 | # col_name   | data_type  | comment  |
 | p| int|  |
 +--++--+

If I apply the current patch, Impala will keep using the old schema even after 
applying the ALTER_TABLE event.
impala> describe my_part_tbl

 +--+--+-+
 | name | type | comment |
 +--+--+-+
 | i| int  | |
 | p| int  | |
 +--+--+-+



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6fd9a9504bf93d2529dc7accbf436ad83e51d8ac
Gerrit-Change-Number: 21019
Gerrit-PatchSet: 3
Gerrit-Owner: Sai Hemanth Gantasala 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 11 Mar 2024 12:27:28 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12693: [DOCS] Typo in link for ltrim in string functions docs

2024-03-11 Thread Daniel Becker (Code Review)
Daniel Becker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21123 )

Change subject: IMPALA-12693: [DOCS] Typo in link for ltrim in string functions 
docs
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21123/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21123/1//COMMIT_MSG@9
PS1, Line 9: f
Nit: should be capitalised: "Fixed".


http://gerrit.cloudera.org:8080/#/c/21123/1//COMMIT_MSG@9
PS1, Line 9: LTRIM
Nit: full stop at the end of the sentence ("LTRIM.").



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If4345fc6d19f04d0c0c6feef3e0c8598271224fe
Gerrit-Change-Number: 21123
Gerrit-PatchSet: 1
Gerrit-Owner: Saurabh Katiyal 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 11 Mar 2024 12:32:00 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12831: Fix HdfsTable.toMinimalTCatalogObject() failed by concurrent modification

2024-03-11 Thread Csaba Ringhofer (Code Review)
Csaba Ringhofer has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21072 )

Change subject: IMPALA-12831: Fix HdfsTable.toMinimalTCatalogObject() failed by 
concurrent modification
..


Patch Set 3: Code-Review+2

upgrading to +2 after Sai's +1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f8e65c0bd24000241eedf8ca765c1e4e14fdb3
Gerrit-Change-Number: 21072
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 11 Mar 2024 15:27:34 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12883: Support updating the charge on an entry in the cache

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21122 )

Change subject: IMPALA-12883: Support updating the charge on an entry in the 
cache
..


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10370/ 
DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34f54fb3a91a77821651c25d8d3bc3a2a3945025
Gerrit-Change-Number: 21122
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kurt Deschler 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Yida Wu 
Gerrit-Comment-Date: Mon, 11 Mar 2024 16:17:34 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12883: Support updating the charge on an entry in the cache

2024-03-11 Thread Joe McDonnell (Code Review)
Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21122 )

Change subject: IMPALA-12883: Support updating the charge on an entry in the 
cache
..


Patch Set 3:

Test failure is not related


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34f54fb3a91a77821651c25d8d3bc3a2a3945025
Gerrit-Change-Number: 21122
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kurt Deschler 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Yida Wu 
Gerrit-Comment-Date: Mon, 11 Mar 2024 16:17:43 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12883: Support updating the charge on an entry in the cache

2024-03-11 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21122 )

Change subject: IMPALA-12883: Support updating the charge on an entry in the 
cache
..


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/21122/3/be/src/util/cache/cache-test.cc
File be/src/util/cache/cache-test.cc:

http://gerrit.cloudera.org:8080/#/c/21122/3/be/src/util/cache/cache-test.cc@180
PS3, Line 180:   // Updating the charge for the erased element evicts something
Why do you call h1 the erased element?


http://gerrit.cloudera.org:8080/#/c/21122/2/be/src/util/cache/cache.h
File be/src/util/cache/cache.h:

http://gerrit.cloudera.org:8080/#/c/21122/2/be/src/util/cache/cache.h@96
PS2, Line 96: // this makes it easier to embed in containers.
> Done
Ack


http://gerrit.cloudera.org:8080/#/c/21122/2/be/src/util/cache/cache.h@130
PS2, Line 130: // this makes it easier to embed in containers.
> Done
Ack


http://gerrit.cloudera.org:8080/#/c/21122/3/be/src/util/cache/lirs-cache-test.cc
File be/src/util/cache/lirs-cache-test.cc:

http://gerrit.cloudera.org:8080/#/c/21122/3/be/src/util/cache/lirs-cache-test.cc@290
PS3, Line 290:   cache_->UpdateCharge(handle, cache_->MaxCharge());
Do you want to validate the handle's charge doesn't change too?


http://gerrit.cloudera.org:8080/#/c/21122/2/be/src/util/cache/lirs-cache.cc
File be/src/util/cache/lirs-cache.cc:

http://gerrit.cloudera.org:8080/#/c/21122/2/be/src/util/cache/lirs-cache.cc@975
PS2, Line 975: handle->set_charge(charge);
> Fixed this and added some tests
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34f54fb3a91a77821651c25d8d3bc3a2a3945025
Gerrit-Change-Number: 21122
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kurt Deschler 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Yida Wu 
Gerrit-Comment-Date: Mon, 11 Mar 2024 18:53:35 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12872: Use Calcite for ...

2024-03-11 Thread Steve Carlin (Code Review)
Hello Impala Public Jenkins,

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

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

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

Change subject: IMPALA-12872: Use Calcite for ...
..

IMPALA-12872: Use Calcite for ...

...query analysis/optimization for simple queries.

This is the first commit to use the Calcite library to parse,
analyze, and optimize queries.

The hook for the planner is through an override of the JniFrontend. The
CalciteJniFrontend class is the driver that walks through each of the
Calcite steps which are as follows:

CalciteQueryParser: Takes the string query and outputs an AST in the
form of Calcite's SqlNode object.

CalciteMetadataHandler: Iterate through the SqlNode from the previous step
and make sure all essential table metadata is retrieved from catalogd.

CalciteValidator: Validate the SqlNode tree, akin to the Impala Analyzer.

CalciteRelNodeConverter: Change the AST into a logical plan. In this first
commit, the only logical nodes used are LogicalTableScan and LogicalProject.
The LogicalTableScan will serve as the node that reads from an Hdfs Table and
the LogicalProject will only project out the used columns in the query. In
later versions, the LogicalProject will also handle function changes.

CalciteOptimizer: This step is to optimize the query. In this cut, it will be
a nop, but in later versions, it will perform logical optimizations via
Calcite's rule mechanism.

CalcitePhysPlanCreator: Converts the Calcite RelNode logical tree into
Impala's PlanNode physical tree

ExecRequestCreator: Implement the existing Impala steps that turn a Single
Node Plan into a Distributed Plan. It will also create the TExecRequest object
needed by the runtime server.

Only some very basic queries will work with this commit. These include:
select * from tbl <-- only needs the LogicalTableScan
select c1 from tbl <-- Also uses the LogicalProject

In the CalciteJniFrontend, there is some basic checks to make sure only
select statements will get processed. Any non-query statement will revert
back to the current Impala planner.

In this iteration, any queries besides the minimal ones listed above will
result in a caught exception which will then be run through the current
Impala planner. The tests that do work can be found in calcite.test and
run through the custom cluster test test_experimental_planner.py

Change-Id: I453fd75b7b705f4d7de1ed73c3e24cafad0b8c98
---
M bin/impala-config.sh
M fe/pom.xml
M fe/src/main/java/org/apache/impala/analysis/TableName.java
M fe/src/main/java/org/apache/impala/planner/PlannerContext.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A java/experimental-planner/pom.xml
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ConvertToImpalaRelRules.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/NodeWithExprs.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/phys/ImpalaHdfsScanNode.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteDb.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCalciteCatalogReader.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteRelNodeConverter.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CompilerStep.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator2.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java
A 
java/exper

[Impala-ASF-CR] IMPALA-12872: Use Calcite for ...

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21109 )

Change subject: IMPALA-12872: Use Calcite for ...
..


Patch Set 4:

(54 comments)

http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java
File 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java:

http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java@88
PS4, Line 88:* location for the column will remain null.
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java@103
PS4, Line 103:   int position = (slotDesc.getColumn().getPosition() + 
nonPartitionedCols) % totalCols;
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java
File 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java:

http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java@40
PS4, Line 40:
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteDb.java
File 
java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteDb.java:

http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteDb.java@50
PS4, Line 50:
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java
File 
java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java:

http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java@67
PS4, Line 67: public class CalciteTable extends RelOptAbstractTable implements 
Table, Prepare.PreparingTable {
line too long (96 > 90)


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java@117
PS4, Line 117:   SlotRef slotref = new 
SlotRef(Path.createRawPath(baseTblRef.getUniqueAlias(), fieldName));
line too long (96 > 90)


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java@122
PS4, Line 122: + "is a complex type (array/map/struct) column. This 
is not currently supported."));
line too long (96 > 90)


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java@181
PS4, Line 181:   @Override
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java@182
PS4, Line 182:   public boolean columnHasDefaultValue(RelDataType rowType, int 
ordinal, InitializerContext initializerContext) {
line too long (113 > 90)


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java@186
PS4, Line 186:   @Override
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCalciteCatalogReader.java
File 
java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCalciteCatalogReader.java:

http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCalciteCatalogReader.java@46
PS4, Line 46: return stmtTableCache;
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java
File 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@53
PS4, Line 53:  * to walk through all the steps of compiling the query (e.g. 
parsing, validating, etc... )
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@58
PS4, Line 58:   protected static final Logger LOG = 
LoggerFactory.getLogger(CalciteJniFrontend.class.getName());
line too long (98 > 90)


http://gerrit.cloudera.org:8080/#/c/21109/4/java/experimental-planner/sr

[Impala-ASF-CR] IMPALA-12872: Use Calcite for ...

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21109 )

Change subject: IMPALA-12872: Use Calcite for ...
..


Patch Set 4:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15475/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I453fd75b7b705f4d7de1ed73c3e24cafad0b8c98
Gerrit-Change-Number: 21109
Gerrit-PatchSet: 4
Gerrit-Owner: Steve Carlin 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 11 Mar 2024 20:03:10 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11499: Refactor UrlEncode function to handle special characters

2024-03-11 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21131 )

Change subject: IMPALA-11499: Refactor UrlEncode function to handle special 
characters
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/21131/1/be/src/util/coding-util.cc
File be/src/util/coding-util.cc:

http://gerrit.cloudera.org:8080/#/c/21131/1/be/src/util/coding-util.cc@47
PS1, Line 47: static function HiveShouldEscape = 
is_any_of("\"#%\\*/:=?\u00FF");
With this change it doesn't look like we need this list or ShouldNotEscape 
anymore, specialCharacterMap covers it.


http://gerrit.cloudera.org:8080/#/c/21131/1/be/src/util/coding-util.cc@84
PS1, Line 84: boost::replace_all(input_str, std::string(1, entry.first), 
entry.second);
So this works because it only replaces specific chars - that won't appear in 
multi-byte characters - rather than anything that's not isalnum?


http://gerrit.cloudera.org:8080/#/c/21131/1/be/src/util/coding-util.cc@95
PS1, Line 95: }
We could return unused capacity with 
https://en.cppreference.com/w/cpp/string/basic_string/shrink_to_fit.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I88c4aba5d811dfcec809583d0c16fcbc0ca730fb
Gerrit-Change-Number: 21131
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Zihao Ye 
Gerrit-Comment-Date: Mon, 11 Mar 2024 20:29:27 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12883: Support updating the charge on an entry in the cache

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21122 )

Change subject: IMPALA-12883: Support updating the charge on an entry in the 
cache
..


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34f54fb3a91a77821651c25d8d3bc3a2a3945025
Gerrit-Change-Number: 21122
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kurt Deschler 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Yida Wu 
Gerrit-Comment-Date: Mon, 11 Mar 2024 20:53:06 +
Gerrit-HasComments: No


[native-toolchain-CR] IMPALA-12886: Bump GoogleTest version to 1.14.0

2024-03-11 Thread Laszlo Gaal (Code Review)
Laszlo Gaal has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/21133


Change subject: IMPALA-12886: Bump GoogleTest version to 1.14.0
..

IMPALA-12886: Bump GoogleTest version to 1.14.0

This is to unblock clang-tidy runs on ARM.
The current version of gtest seems to suffer from a heap corruption
problem that causes unpredictable segfaults in unified-be-tests.

Upgrading to GoogleTest 1.14.0 fixed this problem in test runs.

Change-Id: Iaa52b4809a7c969e863518b182d2df63256f4c43
---
M buildall.sh
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/native-toolchain 
refs/changes/33/21133/1
--
To view, visit http://gerrit.cloudera.org:8080/21133
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaa52b4809a7c969e863518b182d2df63256f4c43
Gerrit-Change-Number: 21133
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 


[Impala-ASF-CR] IMPALA-12872: Use Calcite for ...

2024-03-11 Thread Steve Carlin (Code Review)
Hello Impala Public Jenkins,

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

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

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

Change subject: IMPALA-12872: Use Calcite for ...
..

IMPALA-12872: Use Calcite for ...

...query analysis/optimization for simple queries.

This is the first commit to use the Calcite library to parse,
analyze, and optimize queries.

The hook for the planner is through an override of the JniFrontend. The
CalciteJniFrontend class is the driver that walks through each of the
Calcite steps which are as follows:

CalciteQueryParser: Takes the string query and outputs an AST in the
form of Calcite's SqlNode object.

CalciteMetadataHandler: Iterate through the SqlNode from the previous step
and make sure all essential table metadata is retrieved from catalogd.

CalciteValidator: Validate the SqlNode tree, akin to the Impala Analyzer.

CalciteRelNodeConverter: Change the AST into a logical plan. In this first
commit, the only logical nodes used are LogicalTableScan and LogicalProject.
The LogicalTableScan will serve as the node that reads from an Hdfs Table and
the LogicalProject will only project out the used columns in the query. In
later versions, the LogicalProject will also handle function changes.

CalciteOptimizer: This step is to optimize the query. In this cut, it will be
a nop, but in later versions, it will perform logical optimizations via
Calcite's rule mechanism.

CalcitePhysPlanCreator: Converts the Calcite RelNode logical tree into
Impala's PlanNode physical tree

ExecRequestCreator: Implement the existing Impala steps that turn a Single
Node Plan into a Distributed Plan. It will also create the TExecRequest object
needed by the runtime server.

Only some very basic queries will work with this commit. These include:
select * from tbl <-- only needs the LogicalTableScan
select c1 from tbl <-- Also uses the LogicalProject

In the CalciteJniFrontend, there is some basic checks to make sure only
select statements will get processed. Any non-query statement will revert
back to the current Impala planner.

In this iteration, any queries besides the minimal ones listed above will
result in a caught exception which will then be run through the current
Impala planner. The tests that do work can be found in calcite.test and
run through the custom cluster test test_experimental_planner.py

Change-Id: I453fd75b7b705f4d7de1ed73c3e24cafad0b8c98
---
M bin/impala-config.sh
M fe/pom.xml
M fe/src/main/java/org/apache/impala/analysis/TableName.java
M fe/src/main/java/org/apache/impala/planner/PlannerContext.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A java/experimental-planner/pom.xml
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ConvertToImpalaRelRules.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/NodeWithExprs.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/phys/ImpalaHdfsScanNode.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteDb.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCalciteCatalogReader.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteRelNodeConverter.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CompilerStep.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java
A 
java/experimental-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java
A 
java/experi

[native-toolchain-CR] IMPALA-12697: Set FAIL ON PUBLISH to true by default

2024-03-11 Thread Laszlo Gaal (Code Review)
Laszlo Gaal has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/21134


Change subject: IMPALA-12697: Set FAIL_ON_PUBLISH to true by default
..

IMPALA-12697: Set FAIL_ON_PUBLISH to true by default

Ensure that a package upload failure fails the complete build process
instead of just producing an incomplete toolchain in silence.

This changes the default initialized value of the FAIL_ON_PUBLISH
environment variable to 1. This still allows Jenkins jobs to supply a
different value if it ever becomes necessary.

Change-Id: I156269e8b3b1fa5d743a8ab5a83810001f7dd648
---
M init.sh
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/native-toolchain 
refs/changes/34/21134/1
--
To view, visit http://gerrit.cloudera.org:8080/21134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I156269e8b3b1fa5d743a8ab5a83810001f7dd648
Gerrit-Change-Number: 21134
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 


[native-toolchain-CR] IMPALA-12885: Log configuration values when starting a toolchain build

2024-03-11 Thread Laszlo Gaal (Code Review)
Laszlo Gaal has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/21135


Change subject: IMPALA-12885: Log configuration values when starting a 
toolchain build
..

IMPALA-12885: Log configuration values when starting a toolchain build

Report the main environment variables before the build starts, in a way
similar to what happens at the end of Impala's bin/impala-config.sh

Change-Id: Ie6549bb01acce51771fb796c89826b6bbdc54494
---
M init.sh
1 file changed, 10 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/native-toolchain 
refs/changes/35/21135/1
-- 
To view, visit http://gerrit.cloudera.org:8080/21135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie6549bb01acce51771fb796c89826b6bbdc54494
Gerrit-Change-Number: 21135
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 


[native-toolchain-CR] IMPALA-12697: Set FAIL ON PUBLISH to true by default

2024-03-11 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21134 )

Change subject: IMPALA-12697: Set FAIL_ON_PUBLISH to true by default
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I156269e8b3b1fa5d743a8ab5a83810001f7dd648
Gerrit-Change-Number: 21134
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Mon, 11 Mar 2024 21:13:18 +
Gerrit-HasComments: No


[native-toolchain-CR] IMPALA-12885: Log configuration values when starting a toolchain build

2024-03-11 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21135 )

Change subject: IMPALA-12885: Log configuration values when starting a 
toolchain build
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21135/1/init.sh
File init.sh:

http://gerrit.cloudera.org:8080/#/c/21135/1/init.sh@208
PS1, Line 208: echo "PRODUCTION   = ${PRODUCTION}"
Curious, why does this get curly braces?



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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6549bb01acce51771fb796c89826b6bbdc54494
Gerrit-Change-Number: 21135
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Mon, 11 Mar 2024 21:13:48 +
Gerrit-HasComments: Yes


[native-toolchain-CR] IMPALA-12886: Bump GoogleTest version to 1.14.0

2024-03-11 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21133 )

Change subject: IMPALA-12886: Bump GoogleTest version to 1.14.0
..


Patch Set 1: Code-Review+1

Does this require much work in Impala?


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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa52b4809a7c969e863518b182d2df63256f4c43
Gerrit-Change-Number: 21133
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Mon, 11 Mar 2024 21:14:15 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-11 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 41: Code-Review+1

(7 comments)

http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.h
File be/src/service/workload-management.h:

http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.h@42
PS39, Line 42: /// Name of the database where all workload management tables 
will be stored.
> This is going to allocate storage every place the header is included. Do we
Done


http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.h@51
PS39, Line 51: /// this constant is slightly smaller to keep well away from the 
actual max.
> This should still be extern, or it won't use the right value in all cases.
Done


http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.h@105
PS39, Line 105:   FieldDefinition(const std::string db_col_name, const 
std::string db_col_type,
> These should be `std::move`'d in to place. Right now it's making 2 copies o
Done


http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.cc
File be/src/service/workload-management.cc:

http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.cc@206
PS39, Line 206:
> This is still evaluated during static initialization, before query_log_writ
Done


http://gerrit.cloudera.org:8080/#/c/20770/39/be/src/service/workload-management.cc@354
PS39, Line 354:
> Does this actually need to be a global? Looks like it's only used in this f
Done


http://gerrit.cloudera.org:8080/#/c/20770/41/be/src/service/workload-management.cc
File be/src/service/workload-management.cc:

http://gerrit.cloudera.org:8080/#/c/20770/41/be/src/service/workload-management.cc@186
PS41, Line 186: // Definition of constants/variables declared in 
workload-management.h
nit: this could all be static variables local to this file since they're not 
used elsewhere.


http://gerrit.cloudera.org:8080/#/c/20770/41/be/src/service/workload-management.cc@368
PS41, Line 368:   max_values_length = 
static_cast(FLAGS_query_log_max_query_length) -
We should use this and the batch size to SET MAX_STATEMENT_LENGTH_BYTES for the 
query, and only need to truncate if the total is close to 2GB.

This may benefit from some testing with multiple large queries to ensure 
writing to the table doesn't get overwhelmed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 41
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 11 Mar 2024 21:23:27 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-11 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 41:

(24 comments)

http://gerrit.cloudera.org:8080/#/c/20770/41/be/src/service/workload-management.h
File be/src/service/workload-management.h:

http://gerrit.cloudera.org:8080/#/c/20770/41/be/src/service/workload-management.h@40
PS41, Line 40: wm
Can this renamed with something more explanatory? Perhaps "management" or 
"history"?


http://gerrit.cloudera.org:8080/#/c/20770/41/be/src/service/workload-management.h@102
PS41, Line 102: std::string
Looks like most columns are primitive types. So this can be replaced with 
TPrimitiveType.
Also clarify in comment if these column types are nullable or not.


http://gerrit.cloudera.org:8080/#/c/20770/41/be/src/service/workload-management.h@248
PS41, Line 248: if (ctx.record->base_state->stmt_type == TStmtType::SET) {
  : ctx.sql << "SET";
  :   } else if (ctx.record->base_state->stmt_type == 
TStmtType::QUERY) {
  : ctx.sql << "QUERY";
  :   } else if (ctx.record->base_state->stmt_type == 
TStmtType::DML) {
  : ctx.sql << "DML";
  :   } else if (ctx.record->base_state->stmt_type == 
TStmtType::DDL) {
  : ctx.sql << "DDL";
  :   } else if (ctx.record->base_state->stmt_type == 
TStmtType::EXPLAIN) {
  : ctx.sql << "EXPLAIN";
  :   } else if (ctx.record->base_state->stmt_type == 
TStmtType::ADMIN_FN) {
  : ctx.sql << "ADMIN";
  :   } else if (ctx.record->base_state->stmt_type == 
TStmtType::CONVERT) {
  : ctx.sql << "CONVERT";
  :   } else if (ctx.record->base_state->stmt_type == 
TStmtType::LOAD) {
  : ctx.sql << "LOAD";
  :   } else if (ctx.record->base_state->stmt_type == 
TStmtType::TESTCASE) {
  : ctx.sql << "TESTCASE";
  :   } else {
  : ctx.sql << "N/A";
  :   }
This can be feed directly to StringStream sql. This the how generated 
Types_types.cpp looks like:

const char* _kTStmtTypeNames[] = {  



  "QUERY",  

  
  "DDL",

   
  "DML",



  "EXPLAIN",


 
  "LOAD",   


  
  "SET",


  "ADMIN_FN",   

 
  "TESTCASE",   


  
  "CONVERT",


   
  "UNKNOWN" 

[Impala-ASF-CR] IMPALA-12872: Use Calcite for ...

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21109 )

Change subject: IMPALA-12872: Use Calcite for ...
..


Patch Set 5:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15476/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I453fd75b7b705f4d7de1ed73c3e24cafad0b8c98
Gerrit-Change-Number: 21109
Gerrit-PatchSet: 5
Gerrit-Owner: Steve Carlin 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 11 Mar 2024 21:36:39 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-11 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 41:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20770/41/tests/custom_cluster/test_query_log.py
File tests/custom_cluster/test_query_log.py:

http://gerrit.cloudera.org:8080/#/c/20770/41/tests/custom_cluster/test_query_log.py@87
PS41, Line 87: with_args(
For with_args, consider adding line break after open parentheses.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 41
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 11 Mar 2024 21:36:39 +
Gerrit-HasComments: Yes


[native-toolchain-CR] IMPALA-12885: Log configuration values when starting a toolchain build

2024-03-11 Thread Laszlo Gaal (Code Review)
Laszlo Gaal has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21135 )

Change subject: IMPALA-12885: Log configuration values when starting a 
toolchain build
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21135/1/init.sh
File init.sh:

http://gerrit.cloudera.org:8080/#/c/21135/1/init.sh@208
PS1, Line 208: echo "PRODUCTION   = ${PRODUCTION}"
> Curious, why does this get curly braces?
IIRC that was the first variable I added, the rest was added in a second step. 
IOW pure inconsistency; I'll upload a fix.



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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6549bb01acce51771fb796c89826b6bbdc54494
Gerrit-Change-Number: 21135
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Mon, 11 Mar 2024 22:10:59 +
Gerrit-HasComments: Yes


[native-toolchain-CR] IMPALA-12885: Log configuration values when starting a toolchain build

2024-03-11 Thread Laszlo Gaal (Code Review)
Hello Michael Smith,

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

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

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

Change subject: IMPALA-12885: Log configuration values when starting a 
toolchain build
..

IMPALA-12885: Log configuration values when starting a toolchain build

Report the main environment variables before the build starts, in a way
similar to what happens at the end of Impala's bin/impala-config.sh

Change-Id: Ie6549bb01acce51771fb796c89826b6bbdc54494
---
M init.sh
1 file changed, 10 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/native-toolchain 
refs/changes/35/21135/2
--
To view, visit http://gerrit.cloudera.org:8080/21135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie6549bb01acce51771fb796c89826b6bbdc54494
Gerrit-Change-Number: 21135
Gerrit-PatchSet: 2
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Smith 


[native-toolchain-CR] IMPALA-12885: Log configuration values when starting a toolchain build

2024-03-11 Thread Laszlo Gaal (Code Review)
Laszlo Gaal has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21135 )

Change subject: IMPALA-12885: Log configuration values when starting a 
toolchain build
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21135/1/init.sh
File init.sh:

http://gerrit.cloudera.org:8080/#/c/21135/1/init.sh@208
PS1, Line 208: echo "PRODUCTION   = $PRODUCTION"
> IIRC that was the first variable I added, the rest was added in a second st
Done



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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6549bb01acce51771fb796c89826b6bbdc54494
Gerrit-Change-Number: 21135
Gerrit-PatchSet: 2
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Mon, 11 Mar 2024 22:13:00 +
Gerrit-HasComments: Yes


[native-toolchain-CR] IMPALA-12885: Log configuration values when starting a toolchain build

2024-03-11 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21135 )

Change subject: IMPALA-12885: Log configuration values when starting a 
toolchain build
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6549bb01acce51771fb796c89826b6bbdc54494
Gerrit-Change-Number: 21135
Gerrit-PatchSet: 2
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Mon, 11 Mar 2024 22:26:53 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-11 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 41:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20770/41/tests/custom_cluster/test_query_log.py
File tests/custom_cluster/test_query_log.py:

http://gerrit.cloudera.org:8080/#/c/20770/41/tests/custom_cluster/test_query_log.py@33
PS41, Line 33: class TestQueryLogTable(CustomClusterTestSuite):
 :   """Tests to assert the query log table is correctly 
populated."""
 :
 :   WM_DB = "sys"
 :   QUERY_TBL = "{0}.impala_query_log".format(WM_DB)
 :   PROTOCOL_BEESWAX = ["beeswax"]
 :   PROTOCOL_HS2 = ["hs2"]
 :   PROTOCOL_ALL = [PROTOCOL_BEESWAX[0], PROTOCOL_HS2[0]]
 :
 :   
@CustomClusterTestSuite.with_args(impalad_args="--enable_workload_mgmt "
 :  
"--query_log_write_interval_s=1 "
 :  
"--cluster_id=test_max_select "
 :  
"--shutdown_grace_period_s=10 "
 :  
"--shutdown_deadline_s=60",
 : 
catalogd_args="--enable_workload_mgmt",
 : 
impalad_graceful_shutdown=True)
 :   @pytest.mark.parametrize("client_protocol", PROTOCOL_BEESWAX)
 :   def test_query_log_table_almost_max_select(self, 
client_protocol):
> Impala test framework use ImpalaTestMatrix to parameterize its test functio
Looks like ImpalaTestSuite already has 'protocol' dimension.
https://github.com/apache/impala/blob/70c35425d3f8ac68b23fb8e0d08e12ee763965d7/tests/common/impala_test_suite.py#L185

In that case, TestQueryLogTableHS2Client and TestQueryLogTableAllClients can 
simply override that 'protocol' dimension.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 41
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 11 Mar 2024 22:53:15 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12487: Skip reloading file metadata for ALTER TABLE events with trivial changes in StorageDescriptor

2024-03-11 Thread Sai Hemanth Gantasala (Code Review)
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-12487: Skip reloading file metadata for ALTER_TABLE 
events with trivial changes in StorageDescriptor
..

IMPALA-12487: Skip reloading file metadata for ALTER_TABLE events with
trivial changes in StorageDescriptor

IMPALA-11534 skips reloading file metadata for some trivial ALTER_TABLE
events. However, ALTER_TABLE events that have trivial changes in
StorageDescriptor are not handled in IMPALA-11534. The only changes
that require file metadata reload are: location, rowformat, fileformat,
serde, and storedAsSubDirectories=true. The file metadata reload can be
skipped for all other changes in SD. Also introduced a small
optimization to skip reloading of table schema when ALTER_TABLE changes
location, rowformat, fileformat, and serde in the Storage Descriptor.

Testing:
1) Manual testing by changing SD parameters in local environment.
2) Added unit tests for the same in MetastoreEventsProcessorTest class.

Change-Id: I6fd9a9504bf93d2529dc7accbf436ad83e51d8ac
---
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M 
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
5 files changed, 156 insertions(+), 46 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6fd9a9504bf93d2529dc7accbf436ad83e51d8ac
Gerrit-Change-Number: 21019
Gerrit-PatchSet: 4
Gerrit-Owner: Sai Hemanth Gantasala 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 


[Impala-ASF-CR] IMPALA-12487: Skip reloading file metadata for ALTER TABLE events with trivial changes in StorageDescriptor

2024-03-11 Thread Sai Hemanth Gantasala (Code Review)
Sai Hemanth Gantasala has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21019 )

Change subject: IMPALA-12487: Skip reloading file metadata for ALTER_TABLE 
events with trivial changes in StorageDescriptor
..


Patch Set 4:

(1 comment)

> When testing IMPALA-10976, did you find any test failure if we don't skip 
> reloading table schema?
I have seen some failures but I don't remember now. I think what we can do for 
now is to keep the skipping file metadata reload logic and remove the table 
schema skipping reload logic and we'll address that in IMPALA-10976. Is that ok 
with you?

http://gerrit.cloudera.org:8080/#/c/21019/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:

http://gerrit.cloudera.org:8080/#/c/21019/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1788
PS1, Line 1788:   }
> Thanks for pointing to the existing codes. I found it's an existing issue t
Thanks for pointing out this issue.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6fd9a9504bf93d2529dc7accbf436ad83e51d8ac
Gerrit-Change-Number: 21019
Gerrit-PatchSet: 4
Gerrit-Owner: Sai Hemanth Gantasala 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 11 Mar 2024 22:56:36 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12831: Fix HdfsTable.toMinimalTCatalogObject() failed by concurrent modification

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21072 )

Change subject: IMPALA-12831: Fix HdfsTable.toMinimalTCatalogObject() failed by 
concurrent modification
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f8e65c0bd24000241eedf8ca765c1e4e14fdb3
Gerrit-Change-Number: 21072
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 11 Mar 2024 23:04:56 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12831: Fix HdfsTable.toMinimalTCatalogObject() failed by concurrent modification

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21072 )

Change subject: IMPALA-12831: Fix HdfsTable.toMinimalTCatalogObject() failed by 
concurrent modification
..


Patch Set 4:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10371/ 
DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f8e65c0bd24000241eedf8ca765c1e4e14fdb3
Gerrit-Change-Number: 21072
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 11 Mar 2024 23:04:57 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12831: Fix HdfsTable.toMinimalTCatalogObject() failed by concurrent modification

2024-03-11 Thread Quanlong Huang (Code Review)
Quanlong Huang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21072 )

Change subject: IMPALA-12831: Fix HdfsTable.toMinimalTCatalogObject() failed by 
concurrent modification
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21072/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

http://gerrit.cloudera.org:8080/#/c/21072/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2133
PS1, Line 2133:   /**
> I tried adding a lock for the partition map and it seems not that straightf
Filed IMPALA-12891 for this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f8e65c0bd24000241eedf8ca765c1e4e14fdb3
Gerrit-Change-Number: 21072
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 11 Mar 2024 23:04:21 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12487: Skip reloading file metadata for ALTER TABLE events with trivial changes in StorageDescriptor

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21019 )

Change subject: IMPALA-12487: Skip reloading file metadata for ALTER_TABLE 
events with trivial changes in StorageDescriptor
..


Patch Set 4:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15477/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6fd9a9504bf93d2529dc7accbf436ad83e51d8ac
Gerrit-Change-Number: 21019
Gerrit-PatchSet: 4
Gerrit-Owner: Sai Hemanth Gantasala 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 11 Mar 2024 23:20:29 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-11 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 41:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20770/41/tests/util/workload_management.py
File tests/util/workload_management.py:

http://gerrit.cloudera.org:8080/#/c/20770/41/tests/util/workload_management.py@259
PS41, Line 259:   root_mem = re.search(r'\nPLAN-ROOT 
SINK.*?mem-estimate=(\S+?) mem-reservation',
This doesn't work when I try to write tests using some existing tables, like 
'select * from functional.alltypes'.

I ended up modifying it to

root_mem = re.search(
  r'\n\nF\d+:PLAN FRAGMENT.*?mem-estimate=(\S+?) mem-reservation',
  profile_text, re.DOTALL)

There's a fragment above PLAN-ROOT SINK, and I suspect that's what we actually 
want.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 41
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Tue, 12 Mar 2024 00:03:59 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12626: Capture tables in query for log

2024-03-11 Thread Michael Smith (Code Review)
Hello Andrew Sherman, Jason Fehr, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-12626: Capture tables in query for log
..

IMPALA-12626: Capture tables in query for log

Currently requires 'drop table sys.impala_query_log' to recreate it with
the new column.

Change-Id: I9c9c80b2adf7f3e44225a191fe8eb9df3c4bc5aa
---
M be/src/exec/system-table-scanner.cc
M be/src/service/client-request-state.h
M be/src/service/query-state-record.cc
M be/src/service/query-state-record.h
M be/src/service/workload-management.h
M common/thrift/Frontend.thrift
M common/thrift/SystemTables.thrift
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/custom_cluster/test_query_live.py
M tests/util/workload_management.py
10 files changed, 54 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9c9c80b2adf7f3e44225a191fe8eb9df3c4bc5aa
Gerrit-Change-Number: 20886
Gerrit-PatchSet: 12
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 


[Impala-ASF-CR] IMPALA-12540: Add system.impala query live table

2024-03-11 Thread Michael Smith (Code Review)
Hello Andrew Sherman, Jason Fehr, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-12540: Add system.impala_query_live table
..

IMPALA-12540: Add system.impala_query_live table

Defines SystemTable which are in-memory tables that can provide access
to Impala state. Adds the 'impala_query_live' to the database 'sys',
which already exists for 'sys.impala_query_log'.

Implements the 'impala_query_live' table to view active queries across
all coordinators sharing the same statestore. SystemTables create new
SystemTableScanNodes for their scan node implementation. When computing
scan range locations, SystemTableScanNodes creates a scan range for each
in the cluster (identified via ClusterMembershipMgr). This produces a
plan that looks like:

Query: explain select * from sys.impala_query_live
++
| Explain String |
++
| Max Per-Host Resource Reservation: Memory=4.00MB Threads=2 |
| Per-Host Resource Estimates: Memory=11MB   |
| WARNING: The following tables are missing relevant table   |
| and/or column statistics.  |
| sys.impala_query_live   |
||
| PLAN-ROOT SINK |
| |  |
| 01:EXCHANGE [UNPARTITIONED]|
| |  |
| 00:SCAN SYSTEM_TABLE [sys.impala_query_live]|
|row-size=72B cardinality=20 |
++

Execution will pull data from ImpalaServer on the backend via a
SystemTableScanner implementation based on table name.

In the query profile, SYSTEM_TABLE_SCAN_NODE includes
ActiveQueryCollectionTime and PendingQueryCollectionTime to track time
spent collecting QueryState from ImpalaServer.

Grants QueryScanner private access to ImpalaServer, identical to how
ImpalaHttpHandler access internal server state.

Change-Id: Ie2f9a449f0e5502078931e7f1c5df6e0b762c743
---
M be/src/exec/CMakeLists.txt
M be/src/exec/exec-node.cc
M be/src/exec/scan-node.cc
A be/src/exec/system-table-scan-node.cc
A be/src/exec/system-table-scan-node.h
A be/src/exec/system-table-scanner.cc
A be/src/exec/system-table-scanner.h
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M be/src/scheduling/scheduler.cc
M be/src/service/fe-support.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-state-record.h
M be/src/util/sharded-query-map-util.h
M common/thrift/CMakeLists.txt
M common/thrift/CatalogObjects.thrift
M common/thrift/Descriptors.thrift
M common/thrift/PlanNodes.thrift
M common/thrift/StatestoreService.thrift
A common/thrift/SystemTables.thrift
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
A fe/src/main/java/org/apache/impala/catalog/SystemTable.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SystemTableScanNode.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
A tests/custom_cluster/test_query_live.py
M tests/util/workload_management.py
35 files changed, 1,278 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/20762/29
--
To view, visit http://gerrit.cloudera.org:8080/20762
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2f9a449f0e5502078931e7f1c5df6e0b762c743
Gerrit-Change-Number: 20762
Gerrit-PatchSet: 29
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 


[Impala-ASF-CR] IMPALA-12540: Add system.impala query live table

2024-03-11 Thread Michael Smith (Code Review)
Hello Andrew Sherman, Jason Fehr, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-12540: Add system.impala_query_live table
..

IMPALA-12540: Add system.impala_query_live table

Defines SystemTable which are in-memory tables that can provide access
to Impala state. Adds the 'impala_query_live' to the database 'sys',
which already exists for 'sys.impala_query_log'.

Implements the 'impala_query_live' table to view active queries across
all coordinators sharing the same statestore. SystemTables create new
SystemTableScanNodes for their scan node implementation. When computing
scan range locations, SystemTableScanNodes creates a scan range for each
in the cluster (identified via ClusterMembershipMgr). This produces a
plan that looks like:

Query: explain select * from sys.impala_query_live
++
| Explain String |
++
| Max Per-Host Resource Reservation: Memory=4.00MB Threads=2 |
| Per-Host Resource Estimates: Memory=11MB   |
| WARNING: The following tables are missing relevant table   |
| and/or column statistics.  |
| sys.impala_query_live   |
||
| PLAN-ROOT SINK |
| |  |
| 01:EXCHANGE [UNPARTITIONED]|
| |  |
| 00:SCAN SYSTEM_TABLE [sys.impala_query_live]|
|row-size=72B cardinality=20 |
++

Execution will pull data from ImpalaServer on the backend via a
SystemTableScanner implementation based on table name.

In the query profile, SYSTEM_TABLE_SCAN_NODE includes
ActiveQueryCollectionTime and PendingQueryCollectionTime to track time
spent collecting QueryState from ImpalaServer.

Grants QueryScanner private access to ImpalaServer, identical to how
ImpalaHttpHandler access internal server state.

Change-Id: Ie2f9a449f0e5502078931e7f1c5df6e0b762c743
---
M be/src/exec/CMakeLists.txt
M be/src/exec/exec-node.cc
M be/src/exec/scan-node.cc
A be/src/exec/system-table-scan-node.cc
A be/src/exec/system-table-scan-node.h
A be/src/exec/system-table-scanner.cc
A be/src/exec/system-table-scanner.h
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M be/src/scheduling/scheduler.cc
M be/src/service/fe-support.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-state-record.h
M be/src/util/sharded-query-map-util.h
M common/thrift/CMakeLists.txt
M common/thrift/CatalogObjects.thrift
M common/thrift/Descriptors.thrift
M common/thrift/PlanNodes.thrift
M common/thrift/StatestoreService.thrift
A common/thrift/SystemTables.thrift
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowCreateTableStmt.java
M fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
A fe/src/main/java/org/apache/impala/catalog/SystemTable.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
A fe/src/main/java/org/apache/impala/planner/SystemTableScanNode.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
A tests/custom_cluster/test_query_live.py
M tests/util/workload_management.py
35 files changed, 1,278 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/20762/30
--
To view, visit http://gerrit.cloudera.org:8080/20762
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2f9a449f0e5502078931e7f1c5df6e0b762c743
Gerrit-Change-Number: 20762
Gerrit-PatchSet: 30
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 


[Impala-ASF-CR] IMPALA-12540: Add system.impala query live table

2024-03-11 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20762 )

Change subject: IMPALA-12540: Add system.impala_query_live table
..


Patch Set 30:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.cc
File be/src/exec/system-table-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.cc@19
PS28, Line 19:
> Please #include  since make_unique<> is used
Done


http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.cc@57
PS28, Line 57:
> The docs on this constructor says it should not be used for status that are
Honestly shouldn't be visible, but NOT_IMPLEMENTED_ERROR also seems fine. Done.


http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.cc@57
PS28, Line 57:
> Nit: a message of "Unknown table name" would be clearer.
Done


http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.cc@147
PS28, Line 147:   if (!query.events_timeline_empty()) {
> Since this file does "using namespace boost::algorithm", can the "boost::al
I may try to re-use helpers from workload-management.h, or move this logic to 
QueryStateExpanded.


http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/service/fe-support.cc@651
PS28, Line 651:   vector coordinators;
> Note: TODO
Removed, no clear argument for it.


http://gerrit.cloudera.org:8080/#/c/20762/28/fe/src/main/java/org/apache/impala/planner/SystemTableScanNode.java
File fe/src/main/java/org/apache/impala/planner/SystemTableScanNode.java:

http://gerrit.cloudera.org:8080/#/c/20762/28/fe/src/main/java/org/apache/impala/planner/SystemTableScanNode.java@59
PS28, Line 59: assignConjuncts(analyzer);
> I'll look into it, just copied this from other ScanNodes so I didn't look i
Removed.


http://gerrit.cloudera.org:8080/#/c/20762/28/tests/custom_cluster/test_query_live.py
File tests/custom_cluster/test_query_live.py:

http://gerrit.cloudera.org:8080/#/c/20762/28/tests/custom_cluster/test_query_live.py@34
PS28, Line 34: result1 = self.client.execute("select * from 
functional.alltypes",
> Yes, I'll work on that in the next rebase.
Done


http://gerrit.cloudera.org:8080/#/c/20762/28/tests/custom_cluster/test_query_live.py@409
PS28, Line 409:
> Nit: need space
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2f9a449f0e5502078931e7f1c5df6e0b762c743
Gerrit-Change-Number: 20762
Gerrit-PatchSet: 30
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Tue, 12 Mar 2024 00:18:58 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12626: Capture tables in query for log

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20886 )

Change subject: IMPALA-12626: Capture tables in query for log
..


Patch Set 12:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15479/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c9c80b2adf7f3e44225a191fe8eb9df3c4bc5aa
Gerrit-Change-Number: 20886
Gerrit-PatchSet: 12
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Tue, 12 Mar 2024 00:32:21 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12540: Add system.impala query live table

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20762 )

Change subject: IMPALA-12540: Add system.impala_query_live table
..


Patch Set 29:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15478/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2f9a449f0e5502078931e7f1c5df6e0b762c743
Gerrit-Change-Number: 20762
Gerrit-PatchSet: 29
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Tue, 12 Mar 2024 00:32:40 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12540: Add system.impala query live table

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20762 )

Change subject: IMPALA-12540: Add system.impala_query_live table
..


Patch Set 30:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15480/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2f9a449f0e5502078931e7f1c5df6e0b762c743
Gerrit-Change-Number: 20762
Gerrit-PatchSet: 30
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Tue, 12 Mar 2024 00:42:54 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12831: Fix HdfsTable.toMinimalTCatalogObject() failed by concurrent modification

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21072 )

Change subject: IMPALA-12831: Fix HdfsTable.toMinimalTCatalogObject() failed by 
concurrent modification
..


Patch Set 4: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10371/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f8e65c0bd24000241eedf8ca765c1e4e14fdb3
Gerrit-Change-Number: 21072
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Tue, 12 Mar 2024 04:00:11 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12543: Detect self-events before finishing DDL

2024-03-11 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21029 )

Change subject: IMPALA-12543: Detect self-events before finishing DDL
..


Patch Set 15:

ps15 is just a rebase to pick up IMPALA-12678


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79
Gerrit-Change-Number: 21029
Gerrit-PatchSet: 15
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Tue, 12 Mar 2024 04:03:00 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12771: Impala catalogd events-skipped may mark the wrong number

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21045 )

Change subject: IMPALA-12771: Impala catalogd events-skipped may mark the wrong 
number
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21045/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:

http://gerrit.cloudera.org:8080/#/c/21045/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2347
PS3, Line 2347: 
metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).getCount());
line too long (92 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7aeb04e999b82187eb138c0b643ead259da22f1a
Gerrit-Change-Number: 21045
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Comment-Date: Tue, 12 Mar 2024 05:14:27 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12771: Impala catalogd events-skipped may mark the wrong number

2024-03-11 Thread Anonymous Coward (Code Review)
cclive1...@gmail.com has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/21045 )

Change subject: IMPALA-12771: Impala catalogd events-skipped may mark the wrong 
number
..

IMPALA-12771: Impala catalogd events-skipped may mark the wrong number

The description of events-skipped metric is wrong. Some cases in Add partition
event ,the metric will also be increased, besides for some other cases like 
alter
partition the event is skipped and the log is printed but the events-skipped 
metric
is not increased.

Change-Id: I7aeb04e999b82187eb138c0b643ead259da22f1a
---
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M 
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M 
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
4 files changed, 165 insertions(+), 29 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7aeb04e999b82187eb138c0b643ead259da22f1a
Gerrit-Change-Number: 21045
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 


[Impala-ASF-CR] IMPALA-12771: Impala catalogd events-skipped may mark the wrong number

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21045 )

Change subject: IMPALA-12771: Impala catalogd events-skipped may mark the wrong 
number
..


Patch Set 3:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15481/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7aeb04e999b82187eb138c0b643ead259da22f1a
Gerrit-Change-Number: 21045
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Comment-Date: Tue, 12 Mar 2024 05:38:31 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11499: Refactor UrlEncode function to handle special characters

2024-03-11 Thread Anonymous Coward (Code Review)
pranav.lo...@cloudera.com has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/21131 )

Change subject: IMPALA-11499: Refactor UrlEncode function to handle special 
characters
..

IMPALA-11499: Refactor UrlEncode function to handle special characters

In this commit, a comprehensive mapping of special characters, including
Unicode, to their respective URL-encoded forms has been introduced. The
UrlEncode function has been modified to dynamically iterate through
this mapping, providing a more generic and scalable approach to special
character encoding. This change also enhances code readability and
maintainability by incorporating detailed comments explaining each
section of the updated implementation.

Testing: Some basic tests are provided in unicode-column-name.test.

Change-Id: I88c4aba5d811dfcec809583d0c16fcbc0ca730fb
---
M be/src/util/coding-util.cc
M testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test
2 files changed, 78 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I88c4aba5d811dfcec809583d0c16fcbc0ca730fb
Gerrit-Change-Number: 21131
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Zihao Ye 


[Impala-ASF-CR] IMPALA-11499: Refactor UrlEncode function to handle special characters

2024-03-11 Thread Anonymous Coward (Code Review)
pranav.lo...@cloudera.com has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21131 )

Change subject: IMPALA-11499: Refactor UrlEncode function to handle special 
characters
..


Patch Set 3:

(4 comments)

> Uploaded patch set 3.

http://gerrit.cloudera.org:8080/#/c/21131/1/be/src/util/coding-util.cc
File be/src/util/coding-util.cc:

http://gerrit.cloudera.org:8080/#/c/21131/1/be/src/util/coding-util.cc@47
PS1, Line 47: // Mapping of special characters to their URL-encoded forms
> With this change it doesn't look like we need this list or ShouldNotEscape
Done


http://gerrit.cloudera.org:8080/#/c/21131/1/be/src/util/coding-util.cc@75
PS1, Line 75:   boost::replace_all(input_str, "%", "%25");
> line too long (94 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/21131/1/be/src/util/coding-util.cc@84
PS1, Line 84: // Encode forward slashes to prevent misinterpretation in 
paths or URLs
> So this works because it only replaces specific chars - that won't appear i
Yes, that's correct. The UrlEncode function in the provided code is designed to 
work with UTF-8 encoded strings, where each character can be represented by one 
to four bytes. The key is that it replaces specific characters with their 
URL-encoded forms, and these replacements are carefully chosen to avoid 
interfering with multi-byte characters.The replacements are safe because they 
don't disrupt the structure of multi-byte characters in UTF-8 encoding. The % 
sign followed by two hexadecimal digits, used in URL encoding, ensures that the 
replacements won't be confused with the actual encoding of multi-byte 
characters.

On the other hand, using a generic isalnum check to determine whether a 
character should be replaced might not be suitable for UTF-8 encoded strings. 
It could inadvertently affect multi-byte characters, potentially leading to 
incorrect or corrupted results.


http://gerrit.cloudera.org:8080/#/c/21131/1/be/src/util/coding-util.cc@95
PS1, Line 95: void UrlEncode(const vector& in, string* out, bool 
hive_compat) {
> We could return unused capacity with https://en.cppreference.com/w/cpp/stri
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I88c4aba5d811dfcec809583d0c16fcbc0ca730fb
Gerrit-Change-Number: 21131
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Zihao Ye 
Gerrit-Comment-Date: Tue, 12 Mar 2024 05:56:40 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-11499: Refactor UrlEncode function to handle special characters

2024-03-11 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21131 )

Change subject: IMPALA-11499: Refactor UrlEncode function to handle special 
characters
..


Patch Set 3:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15482/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I88c4aba5d811dfcec809583d0c16fcbc0ca730fb
Gerrit-Change-Number: 21131
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Zihao Ye 
Gerrit-Comment-Date: Tue, 12 Mar 2024 06:22:06 +
Gerrit-HasComments: No