You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/06/02 14:10:03 UTC
[GitHub] [hive] scarlin-cloudera opened a new pull request #2342: HIVE-25189: Fetch validWriteIdList for tables before table request.
scarlin-cloudera opened a new pull request #2342:
URL: https://github.com/apache/hive/pull/2342
Added code to ensure the validWriteIdList for all tables are fetched
before any table request is done.
The tables are now gathered at parsing time within FromClauseParser.g.
One of the initial steps while parsing is to make an HMS call to fetch
the validWriteIdList for all parsed tables. This is done within a newly
added class called CacheTableHelper. The only purpose of this class is
to populate the query HMS cache for future calls to HMS. The methods to
this class do not contain any return values.
One complication to sending the request up front is when the query contains
views. The views will have underlying physical tables that need fetching but
we won't know these tables until after doing an HMS call. This is handled
by using the underlying tables of the most recent time a query used this view.
The idea here is that views don't change very often, so we will most likely
be able to cache these tables by guessing that these tables are still in the
view. On the chance that the view did change, we may be doing a fetch of
validWriteIdLists on tables no longer used or we may miss tables added on
the new definition of the view. If that happens, the only downside is an
extra HMS call as we get the updated information of the view (which cannot
be optimized). The view will then be updated so that the next query will
be optimized.
Because this view information is cached at a server level, a cap of 10000
tables to track was used. Also, on the low chance there is a problem with
memory consumption on this feature, it can be turned off via the
HIVE_OPTIMIZE_VIEW_CACHE_ENABLED conf variable.
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
The fetch of validWriteIdList from HMS will be done at the beginning of the query with all parsed tables.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Slight performance boost. The get_table_req will only have to be done once with the validWriteIdList. Also, all validWriteIdLists will be batched together in one HMS call.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Run through unit tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz commented on a change in pull request #2342: HIVE-25189: Fetch validWriteIdList for tables before table request.
Posted by GitBox <gi...@apache.org>.
kasakrisz commented on a change in pull request #2342:
URL: https://github.com/apache/hive/pull/2342#discussion_r644466843
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/CacheTableHelper.java
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.ql.parse;
+
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Queue;
+import java.util.Set;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.commons.lang3.tuple.ImmutablePair;
+import org.apache.hadoop.hive.common.ValidTxnList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
+import org.apache.hadoop.hive.ql.lockmgr.HiveTxnManager;
+import org.apache.hadoop.hive.ql.session.SessionState;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class to help populate the cache at the beginning of query analysis. We would like
+ * to minimize the number of calls to fetch validWriteIdLists from the metastore. HMS
+ * has an API to request this object for multiple tables within one call, and this class
+ * uses that API.
+ *
+ * The sole purpose of this class is to help populate the HMS query cache. Nothing is returned
+ * from the public methods. In this way, if another method attempts to fetch a validWriteIdList,
+ * the SessionHiveMetaStoreClient query cache will contain the information.
+ *
+ * Because this class is only responsible for cache population, it is not a requirement for
+ * a caller to supply all the tables necessary for the query. It is also not a requirement
+ * for the tables to be part of the query. Of course, the query qill benefit if those
+ * conditions were true, but if the table is not in the cache, a later call fetching the writeids
+ * will hit the HMS server and will not fail.
+ *
+ * One tricky aspect to this class is that if a view is passed in, we want to fetch the
+ * validWriteIdLists for the underlying tables. At the beginning of the query, it is impossible
+ * to know the underlying tables without contacting HMS.
+ *
+ * In order to handle the underlying tables to the views, we keep a cache that holds our
+ * best guess. If we see a view in any query, we set up a server-wide cache that tracks
+ * the underlying tables to the view. If the view doesn't change, then this information
+ * will be accurate and allow us to fetch the underlying tables on our next query. If the
+ * view does change and the underlying tables are different, our fetch won't retrieve the
+ * correct information. But that's ok...remember what was said earlier that it is not
+ * a requirement for the tables to be part of the query. Later on in the query, this class
+ * will be called on the view level via the populateCacheForView call. At that point, if
+ * something changed, it will populate the cache with the newly detected tables. It will also
+ * change the underying table information for the view to optimize the next query using
+ * the view.
+ */
+public class CacheTableHelper {
+ protected static final Logger LOG = LoggerFactory.getLogger(CacheTableHelper.class);
+
+ // Server wide cache used to hold what we currently think are the underlying tables
+ // for a view (which is the key). This information can go stale, but that's ok. The only
+ // repercussion of a stale view is that we will have to make an additional HMS call
+ // to retrieve the validWriteIdList for the changed tables.
+ // Will hold 10000 objects, can't imagine that being more than a couple of M, tops.
+ private static final Cache<String, Set<String>> underlyingTableHints = Caffeine.newBuilder()
+ .maximumSize(10000)
+ .build();
+
+ /**
+ * Populates the cache for the given table pairs. The tables passed in are a Pair
+ * containing the dbname (null if not given) and the table name.
+ */
+ public void populateCache(List<Pair<String, String>> tables, HiveConf conf,
+ HiveTxnManager txnMgr) {
+ // if there is no transaction, then we don't need to fetch the validWriteIds.
+ if (txnMgr == null || !txnMgr.isTxnOpen()) {
+ return;
+ }
+
+ List<String> fullTableNamesList = new ArrayList<>(getAllUniqueViewsAndTables(tables));
+ LOG.debug("Populating query cache");
+ for (String s : fullTableNamesList) {
+ LOG.debug("Populating table " + s);
+ }
+ String validTxnList = conf.get(ValidTxnList.VALID_TXNS_KEY);
+ try {
+ txnMgr.getValidWriteIds(fullTableNamesList, validTxnList);
+ } catch (Exception e) {
+ LOG.info("Population of valid write id list cache failed, will be done later in query.");
+ }
+ }
+
+ /**
+ * Populates the cache for the given table pairs associated with a viewName.
+ * If the table names provided match the table names that we think are associated
+ * with the view, we just return, because we presume that they have already been
+ * popuated via the "populateCache" method. If they are different, we populate the cache
+ * with the new associated tables and change our associated tables for the view.
+ */
+ public void populateCacheForView(List<Pair<String, String>> tables, HiveConf conf,
+ HiveTxnManager txnMgr, String viewName) {
+ if (!conf.getBoolVar(ConfVars.HIVE_OPTIMIZE_VIEW_CACHE_ENABLED)) {
+ return;
+ }
+
+ // The viewname passed in is of the form "view@table", so we replace the "@" with
+ // a ".".
+ viewName = viewName.replace("@", ".");
Review comment:
Hive enables `@` in table/view names. Could you please check if this code behaves as expected?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on pull request #2342: HIVE-25189: Fetch validWriteIdList for tables before table request.
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2342:
URL: https://github.com/apache/hive/pull/2342#issuecomment-856458040
@deniskuzZ: if I understand correctly, then here we have collected every table used by the query in the first phase of the compilation. If my understanding is correct, then we have a good place to request the locks for the tables and prevent / reduce race conditions during the compilation.
@scarlin-cloudera, @deniskuzZ what do you think?
Thanks, Peter
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz commented on a change in pull request #2342: HIVE-25189: Fetch validWriteIdList for tables before table request.
Posted by GitBox <gi...@apache.org>.
kasakrisz commented on a change in pull request #2342:
URL: https://github.com/apache/hive/pull/2342#discussion_r644466843
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/CacheTableHelper.java
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.ql.parse;
+
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Queue;
+import java.util.Set;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.commons.lang3.tuple.ImmutablePair;
+import org.apache.hadoop.hive.common.ValidTxnList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
+import org.apache.hadoop.hive.ql.lockmgr.HiveTxnManager;
+import org.apache.hadoop.hive.ql.session.SessionState;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class to help populate the cache at the beginning of query analysis. We would like
+ * to minimize the number of calls to fetch validWriteIdLists from the metastore. HMS
+ * has an API to request this object for multiple tables within one call, and this class
+ * uses that API.
+ *
+ * The sole purpose of this class is to help populate the HMS query cache. Nothing is returned
+ * from the public methods. In this way, if another method attempts to fetch a validWriteIdList,
+ * the SessionHiveMetaStoreClient query cache will contain the information.
+ *
+ * Because this class is only responsible for cache population, it is not a requirement for
+ * a caller to supply all the tables necessary for the query. It is also not a requirement
+ * for the tables to be part of the query. Of course, the query qill benefit if those
+ * conditions were true, but if the table is not in the cache, a later call fetching the writeids
+ * will hit the HMS server and will not fail.
+ *
+ * One tricky aspect to this class is that if a view is passed in, we want to fetch the
+ * validWriteIdLists for the underlying tables. At the beginning of the query, it is impossible
+ * to know the underlying tables without contacting HMS.
+ *
+ * In order to handle the underlying tables to the views, we keep a cache that holds our
+ * best guess. If we see a view in any query, we set up a server-wide cache that tracks
+ * the underlying tables to the view. If the view doesn't change, then this information
+ * will be accurate and allow us to fetch the underlying tables on our next query. If the
+ * view does change and the underlying tables are different, our fetch won't retrieve the
+ * correct information. But that's ok...remember what was said earlier that it is not
+ * a requirement for the tables to be part of the query. Later on in the query, this class
+ * will be called on the view level via the populateCacheForView call. At that point, if
+ * something changed, it will populate the cache with the newly detected tables. It will also
+ * change the underying table information for the view to optimize the next query using
+ * the view.
+ */
+public class CacheTableHelper {
+ protected static final Logger LOG = LoggerFactory.getLogger(CacheTableHelper.class);
+
+ // Server wide cache used to hold what we currently think are the underlying tables
+ // for a view (which is the key). This information can go stale, but that's ok. The only
+ // repercussion of a stale view is that we will have to make an additional HMS call
+ // to retrieve the validWriteIdList for the changed tables.
+ // Will hold 10000 objects, can't imagine that being more than a couple of M, tops.
+ private static final Cache<String, Set<String>> underlyingTableHints = Caffeine.newBuilder()
+ .maximumSize(10000)
+ .build();
+
+ /**
+ * Populates the cache for the given table pairs. The tables passed in are a Pair
+ * containing the dbname (null if not given) and the table name.
+ */
+ public void populateCache(List<Pair<String, String>> tables, HiveConf conf,
+ HiveTxnManager txnMgr) {
+ // if there is no transaction, then we don't need to fetch the validWriteIds.
+ if (txnMgr == null || !txnMgr.isTxnOpen()) {
+ return;
+ }
+
+ List<String> fullTableNamesList = new ArrayList<>(getAllUniqueViewsAndTables(tables));
+ LOG.debug("Populating query cache");
+ for (String s : fullTableNamesList) {
+ LOG.debug("Populating table " + s);
+ }
+ String validTxnList = conf.get(ValidTxnList.VALID_TXNS_KEY);
+ try {
+ txnMgr.getValidWriteIds(fullTableNamesList, validTxnList);
+ } catch (Exception e) {
+ LOG.info("Population of valid write id list cache failed, will be done later in query.");
+ }
+ }
+
+ /**
+ * Populates the cache for the given table pairs associated with a viewName.
+ * If the table names provided match the table names that we think are associated
+ * with the view, we just return, because we presume that they have already been
+ * popuated via the "populateCache" method. If they are different, we populate the cache
+ * with the new associated tables and change our associated tables for the view.
+ */
+ public void populateCacheForView(List<Pair<String, String>> tables, HiveConf conf,
+ HiveTxnManager txnMgr, String viewName) {
+ if (!conf.getBoolVar(ConfVars.HIVE_OPTIMIZE_VIEW_CACHE_ENABLED)) {
+ return;
+ }
+
+ // The viewname passed in is of the form "view@table", so we replace the "@" with
+ // a ".".
+ viewName = viewName.replace("@", ".");
Review comment:
Hive enables `@` in table/view names. Could you please check if this code behaves as expected?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] scarlin-cloudera commented on a change in pull request #2342: HIVE-25189: Fetch validWriteIdList for tables before table request.
Posted by GitBox <gi...@apache.org>.
scarlin-cloudera commented on a change in pull request #2342:
URL: https://github.com/apache/hive/pull/2342#discussion_r644842695
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/CacheTableHelper.java
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.ql.parse;
+
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Queue;
+import java.util.Set;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.commons.lang3.tuple.ImmutablePair;
+import org.apache.hadoop.hive.common.ValidTxnList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
+import org.apache.hadoop.hive.ql.lockmgr.HiveTxnManager;
+import org.apache.hadoop.hive.ql.session.SessionState;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class to help populate the cache at the beginning of query analysis. We would like
+ * to minimize the number of calls to fetch validWriteIdLists from the metastore. HMS
+ * has an API to request this object for multiple tables within one call, and this class
+ * uses that API.
+ *
+ * The sole purpose of this class is to help populate the HMS query cache. Nothing is returned
+ * from the public methods. In this way, if another method attempts to fetch a validWriteIdList,
+ * the SessionHiveMetaStoreClient query cache will contain the information.
+ *
+ * Because this class is only responsible for cache population, it is not a requirement for
+ * a caller to supply all the tables necessary for the query. It is also not a requirement
+ * for the tables to be part of the query. Of course, the query qill benefit if those
+ * conditions were true, but if the table is not in the cache, a later call fetching the writeids
+ * will hit the HMS server and will not fail.
+ *
+ * One tricky aspect to this class is that if a view is passed in, we want to fetch the
+ * validWriteIdLists for the underlying tables. At the beginning of the query, it is impossible
+ * to know the underlying tables without contacting HMS.
+ *
+ * In order to handle the underlying tables to the views, we keep a cache that holds our
+ * best guess. If we see a view in any query, we set up a server-wide cache that tracks
+ * the underlying tables to the view. If the view doesn't change, then this information
+ * will be accurate and allow us to fetch the underlying tables on our next query. If the
+ * view does change and the underlying tables are different, our fetch won't retrieve the
+ * correct information. But that's ok...remember what was said earlier that it is not
+ * a requirement for the tables to be part of the query. Later on in the query, this class
+ * will be called on the view level via the populateCacheForView call. At that point, if
+ * something changed, it will populate the cache with the newly detected tables. It will also
+ * change the underying table information for the view to optimize the next query using
+ * the view.
+ */
+public class CacheTableHelper {
+ protected static final Logger LOG = LoggerFactory.getLogger(CacheTableHelper.class);
+
+ // Server wide cache used to hold what we currently think are the underlying tables
+ // for a view (which is the key). This information can go stale, but that's ok. The only
+ // repercussion of a stale view is that we will have to make an additional HMS call
+ // to retrieve the validWriteIdList for the changed tables.
+ // Will hold 10000 objects, can't imagine that being more than a couple of M, tops.
+ private static final Cache<String, Set<String>> underlyingTableHints = Caffeine.newBuilder()
+ .maximumSize(10000)
+ .build();
+
+ /**
+ * Populates the cache for the given table pairs. The tables passed in are a Pair
+ * containing the dbname (null if not given) and the table name.
+ */
+ public void populateCache(List<Pair<String, String>> tables, HiveConf conf,
+ HiveTxnManager txnMgr) {
+ // if there is no transaction, then we don't need to fetch the validWriteIds.
+ if (txnMgr == null || !txnMgr.isTxnOpen()) {
+ return;
+ }
+
+ List<String> fullTableNamesList = new ArrayList<>(getAllUniqueViewsAndTables(tables));
+ LOG.debug("Populating query cache");
+ for (String s : fullTableNamesList) {
+ LOG.debug("Populating table " + s);
+ }
+ String validTxnList = conf.get(ValidTxnList.VALID_TXNS_KEY);
+ try {
+ txnMgr.getValidWriteIds(fullTableNamesList, validTxnList);
+ } catch (Exception e) {
+ LOG.info("Population of valid write id list cache failed, will be done later in query.");
+ }
+ }
+
+ /**
+ * Populates the cache for the given table pairs associated with a viewName.
+ * If the table names provided match the table names that we think are associated
+ * with the view, we just return, because we presume that they have already been
+ * popuated via the "populateCache" method. If they are different, we populate the cache
+ * with the new associated tables and change our associated tables for the view.
+ */
+ public void populateCacheForView(List<Pair<String, String>> tables, HiveConf conf,
+ HiveTxnManager txnMgr, String viewName) {
+ if (!conf.getBoolVar(ConfVars.HIVE_OPTIMIZE_VIEW_CACHE_ENABLED)) {
+ return;
+ }
+
+ // The viewname passed in is of the form "view@table", so we replace the "@" with
+ // a ".".
+ viewName = viewName.replace("@", ".");
Review comment:
Fixed. Comment should also have said "dbname@table"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] scarlin-cloudera commented on a change in pull request #2342: HIVE-25189: Fetch validWriteIdList for tables before table request.
Posted by GitBox <gi...@apache.org>.
scarlin-cloudera commented on a change in pull request #2342:
URL: https://github.com/apache/hive/pull/2342#discussion_r644842695
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/CacheTableHelper.java
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.ql.parse;
+
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Queue;
+import java.util.Set;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.commons.lang3.tuple.ImmutablePair;
+import org.apache.hadoop.hive.common.ValidTxnList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
+import org.apache.hadoop.hive.ql.lockmgr.HiveTxnManager;
+import org.apache.hadoop.hive.ql.session.SessionState;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class to help populate the cache at the beginning of query analysis. We would like
+ * to minimize the number of calls to fetch validWriteIdLists from the metastore. HMS
+ * has an API to request this object for multiple tables within one call, and this class
+ * uses that API.
+ *
+ * The sole purpose of this class is to help populate the HMS query cache. Nothing is returned
+ * from the public methods. In this way, if another method attempts to fetch a validWriteIdList,
+ * the SessionHiveMetaStoreClient query cache will contain the information.
+ *
+ * Because this class is only responsible for cache population, it is not a requirement for
+ * a caller to supply all the tables necessary for the query. It is also not a requirement
+ * for the tables to be part of the query. Of course, the query qill benefit if those
+ * conditions were true, but if the table is not in the cache, a later call fetching the writeids
+ * will hit the HMS server and will not fail.
+ *
+ * One tricky aspect to this class is that if a view is passed in, we want to fetch the
+ * validWriteIdLists for the underlying tables. At the beginning of the query, it is impossible
+ * to know the underlying tables without contacting HMS.
+ *
+ * In order to handle the underlying tables to the views, we keep a cache that holds our
+ * best guess. If we see a view in any query, we set up a server-wide cache that tracks
+ * the underlying tables to the view. If the view doesn't change, then this information
+ * will be accurate and allow us to fetch the underlying tables on our next query. If the
+ * view does change and the underlying tables are different, our fetch won't retrieve the
+ * correct information. But that's ok...remember what was said earlier that it is not
+ * a requirement for the tables to be part of the query. Later on in the query, this class
+ * will be called on the view level via the populateCacheForView call. At that point, if
+ * something changed, it will populate the cache with the newly detected tables. It will also
+ * change the underying table information for the view to optimize the next query using
+ * the view.
+ */
+public class CacheTableHelper {
+ protected static final Logger LOG = LoggerFactory.getLogger(CacheTableHelper.class);
+
+ // Server wide cache used to hold what we currently think are the underlying tables
+ // for a view (which is the key). This information can go stale, but that's ok. The only
+ // repercussion of a stale view is that we will have to make an additional HMS call
+ // to retrieve the validWriteIdList for the changed tables.
+ // Will hold 10000 objects, can't imagine that being more than a couple of M, tops.
+ private static final Cache<String, Set<String>> underlyingTableHints = Caffeine.newBuilder()
+ .maximumSize(10000)
+ .build();
+
+ /**
+ * Populates the cache for the given table pairs. The tables passed in are a Pair
+ * containing the dbname (null if not given) and the table name.
+ */
+ public void populateCache(List<Pair<String, String>> tables, HiveConf conf,
+ HiveTxnManager txnMgr) {
+ // if there is no transaction, then we don't need to fetch the validWriteIds.
+ if (txnMgr == null || !txnMgr.isTxnOpen()) {
+ return;
+ }
+
+ List<String> fullTableNamesList = new ArrayList<>(getAllUniqueViewsAndTables(tables));
+ LOG.debug("Populating query cache");
+ for (String s : fullTableNamesList) {
+ LOG.debug("Populating table " + s);
+ }
+ String validTxnList = conf.get(ValidTxnList.VALID_TXNS_KEY);
+ try {
+ txnMgr.getValidWriteIds(fullTableNamesList, validTxnList);
+ } catch (Exception e) {
+ LOG.info("Population of valid write id list cache failed, will be done later in query.");
+ }
+ }
+
+ /**
+ * Populates the cache for the given table pairs associated with a viewName.
+ * If the table names provided match the table names that we think are associated
+ * with the view, we just return, because we presume that they have already been
+ * popuated via the "populateCache" method. If they are different, we populate the cache
+ * with the new associated tables and change our associated tables for the view.
+ */
+ public void populateCacheForView(List<Pair<String, String>> tables, HiveConf conf,
+ HiveTxnManager txnMgr, String viewName) {
+ if (!conf.getBoolVar(ConfVars.HIVE_OPTIMIZE_VIEW_CACHE_ENABLED)) {
+ return;
+ }
+
+ // The viewname passed in is of the form "view@table", so we replace the "@" with
+ // a ".".
+ viewName = viewName.replace("@", ".");
Review comment:
Fixed. Comment should also have said "dbname@table"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz merged pull request #2342: HIVE-25189: Fetch validWriteIdList for tables before table request.
Posted by GitBox <gi...@apache.org>.
kasakrisz merged pull request #2342:
URL: https://github.com/apache/hive/pull/2342
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org