TAJO-1430: Improve SQLAnalyzer by session-based parsing-result caching #442

dongjoon-hyun · 2015-03-20T07:55:57Z

Please see the issue TAJO-1430 for the effect.

dongjoon-hyun · 2015-03-24T08:57:36Z

The cache is maintained by approximately LRU manner with 200 size and also cached item will be expired after 1 hour since last access.

session.setQueryCache(CacheBuilder.newBuilder()
          .maximumSize(200)
          .expireAfterAccess(1, TimeUnit.HOURS)
          .build(new CacheLoader<String, Expr>() {
            public Expr load(String sql) throws SQLSyntaxError {
              return analyzer.parse(sql);
            }
      })
);

dongjoon-hyun · 2015-03-25T02:24:50Z

Rebased.

dongjoon-hyun · 2015-04-04T09:46:11Z

Rebased.

dongjoon-hyun · 2015-04-05T03:45:31Z

Hmm. I attached the test pass result of the following command.

mvn clean install -Pparallel-test,hcatalog-0.12.0 -DLOG_LEVEL=ERROR -Dmaven.fork.count=2 > TAJO-1430.travis.log.txt

https://app.box.com/s/rx85b6pv26sgo400e5dxf30bepyr86nl

Currently, Travis CI seems to be unstable and the builds fail on other reasons like the following.

ERROR: org.apache.tajo.master.rm.TajoWorkerResourceManager (run(346)) - java.lang.InterruptedException
2015-04-04 10:12:39,174 ERROR: org.apache.tajo.util.history.HistoryWriter (writeHistory(318)) - Error while saving query history: q_1428141094535_0543:Filesystem closed

jihoonson · 2015-04-05T16:08:49Z

tajo-core/src/main/java/org/apache/tajo/master/GlobalEngine.java

@@ -155,7 +174,7 @@ public SubmitQueryResponse executeQuery(Session session, String query, boolean i
      if (isJson) {
        planningContext = buildExpressionFromJson(query);


It would be better if the cache is supported for json queries.

Is it slow?

Honestly, I didn't estimate the exact time. But, using cache will be much faster than parsing huge json queries, I think.

jihoonson · 2015-04-05T16:26:12Z

Thanks for the contribution.
I left some comments.

dongjoon-hyun · 2015-04-05T22:49:12Z

Thank you, @jihoonson . By the way, could you see the Jira, too? There was some discussion about this.

https://issues.apache.org/jira/browse/TAJO-1430

It's just I want to make sure what I understand.
By the way, sorry for the outdated title of this pull request outdate.
I'll update now according to the Jira title.

dongjoon-hyun · 2015-04-06T07:05:39Z

@jihoonson , I changed this patch according to your advice except JSON.
I'm still not sure if JSON parser has performance issue. I think it's a little bit beyond this issue's scope.

jihoonson · 2015-04-06T15:21:24Z

tajo-core/src/main/java/org/apache/tajo/master/GlobalEngine.java

+    // Set queryCache in session
+    if (session.getQueryCache() == null) {
+      session.setQueryCache(CacheBuilder.newBuilder()
+        .maximumSize(200)


Here, the size means the number of items contained in the cache. So, the actual cache size depends on the size of cached queries. This will have a problem of potentially exhausting memory. So, it would be better to set the maximum weight rather than the maximum size.

In addition, I think that the maximum weight and the expiration period should be configurable. ConfVar would be a good place because we don't need to maintain different configurations for each user.

That's a good idea. No problem. By the way, the weight should be estimated by SQL query string length, right? Or, do you have some Util to measure Expr memory consumption? Up to now, I can not find that kind of code in Expr. Do you think SQL query string length is okay?

jihoonson · 2015-04-06T15:22:24Z

Ok. I'll investigate the json parsing performance. If you have any references for that, please share with me.

I left another comment.
Thanks.

dongjoon-hyun · 2015-04-06T22:16:24Z

In fact, I don't know any use cases of JSON in the real fields. Is it popular to use JSON query?

jihoonson · 2015-04-07T00:42:26Z

We support json queries to address some requests to integrate other olap tools. As you know, generating json is much easier than generating sql.

On the weight, the query length looks good.

dongjoon-hyun · 2015-04-07T02:20:57Z

According to your advice, I updated and finished testing the code. In these days, Travis CI is prone to fail. If you need, I will upload my tested log.

dongjoon-hyun · 2015-04-07T04:19:49Z

Travis CI fails. Here is my succeeded test log.

mvn clean install -Pparallel-test,hcatalog-0.12.0 -DLOG_LEVEL=WARN -Dmaven.fork.count=2 > TAJO-1430.150407.log.txt

https://app.box.com/s/s7ivihn6myvpr2sbr0t6cfd8fb55zawj

jihoonson · 2015-04-07T05:02:26Z

Thanks. Would you mind triggering Jenkins by putting your patch on Jira?
Jenkins is better than Travis because it also includes the Findbugs test.

dongjoon-hyun · 2015-04-07T05:36:07Z

Sure!

dongjoon-hyun · 2015-04-07T06:32:11Z

Hi, @jihoonson . Jenkins fails to start the test.

Compiling /home/jenkins/jenkins-slave/workspace/PreCommit-TAJO-Build/incubator-tajo
/home/jenkins/tools/maven/latest/bin/mvn clean test -DskipTests -Phcatalog-0.12.0 > /home/jenkins/jenkins-slave/workspace/PreCommit-TAJO-Build/patchprocess/trunkJavacWarnings.txt 2>&1
Trunk compilation is broken?

What can I do now? Please visit the following link for detail info.

https://builds.apache.org/job/PreCommit-TAJO-Build/719/console

dongjoon-hyun · 2015-04-07T06:36:19Z

Hmm. Let me check for a second. I found the following Jenkins logs.

[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-TAJO-Build/incubator-tajo/tajo-core/src/main/java/org/apache/tajo/master/GlobalEngine.java:[160,13] setQueryCache(com.google.common.cache.LoadingCache<java.lang.String,org.apache.tajo.algebra.Expr>) in org.apache.tajo.session.Session cannot be applied to (com.google.common.cache.LoadingCache<java.lang.Object,org.apache.tajo.algebra.Expr>)
[INFO] 1 error

dongjoon-hyun · 2015-04-07T07:23:07Z

For JDK6, I split one function call line into two lines.

jihoonson · 2015-04-12T10:06:35Z

tajo-common/src/main/java/org/apache/tajo/conf/TajoConf.java

@@ -203,6 +203,7 @@ public static int setDateOrder(int dateOrder) {

    // Query Configuration
    QUERY_SESSION_TIMEOUT("tajo.query.session.timeout-sec", 60, Validators.min("0")),
+    QUERY_SESSION_CACHE_SIZE("tajo.query.session.cache-size", 1000000, Validators.min("1000000")),


I have a couple of comments on this line.

The cache size seems to be specified in bytes. This will be difficult for humans to specify the exact cache size. How about using the size unit of KB?

The session variable name looks too general even though this cache is only for parsed queries. It would be better if users can figure out what will be configured from its name. Also, the size unit should be included in the name like tajo.task.size-mb.

The minimum cache size is 1 GB. Do you have any reasons?

In addition to the size configuration, it would be great if users can turn off/on this cache feature. This is because the cache may be useless in some workloads such as ad-hoc analysis.

Hi, @jihoonson . Thank you for kind advice.

What about tajo.query.session.query-cache-size-mb, then? I think it would be more proper according to your advice.

The minimum cache size is 1MB in terms of SQL String length. As you see in TAJO-1430 example, 100K-length query takes over 30 seconds in SQL parsing, so the cache will hold up to 10 x 100K queries accessed in last 1 hour. In real situation, 10 or more MB is needed.

For the cache on/off feautre, is it okay by using the condition tajo.query.session.query-cache-size-mb=0?

Oh, right. I meant 1MB not 1GB. According to your comment, it takes quite long time to parse queries even though their size is a few KBs. So, I think that KB is appropriate for the unit of size.

On minimum cache size, Validators.min("1000000") means that the cache size under 1MB is an invalid value for this configuration. I think that you didn't intend that, but if so, please tell me the reason.

On the cache on/off, your suggestion looks good. In addition to your suggestion, it will be good if we can avoid checking the cache when the configured cache size is 0. I think that this cache will not be used in many cases because the cached data can be used only when the exactly same queries are submitted repeatedly.

tajo.query.session.query-cache-size-kb, then? No problem.

On minimum cache size, let say 1K-size cache holding 10 x 100Byte SQL queries. The effect is small. In this case, we had better off the cache. In that sense, we need to guide the effective cache size practically.

If cache is off, the cache will be null and Tajo will avoid checking the cache. I agree that it's important.

This query cache will be important especially for PreparedStatement of TAJO-1435. (I'm working on this too.) PreparedStatement is popular in real enterprise environments. I did add PLACE_HOLDER in SQL syntax for '?' of PreparedStatement and am trying to replace them after cloning Expr. (Anyway, this is beyond the scope of this issue).

Thanks for your rapid reply. Here are my answers.

Thanks for understanding.

I mean, with Validators.min("1000000"), Tajo won't accept tajo.query.session.query-cache-size-mb=0. Validators should be used to validate the configuration. So, IMO, Validators.min(0) is better to prohibit negative values.

Thanks for understanding.

I also think that query caching is important. I mean, the current implementation will not be popularly used because the cached data can be used only when the exactly same queries are submitted repeatedly.

You're right. Validators.min(0) is proper now because we use 0 for cache off option, too.

I see. Current implementation does.
Now, I will update the code soon. Thank you for reviewing, always!

Thanks for your understanding!

dongjoon-hyun · 2015-04-12T15:54:55Z

I changed the code and finished testing on my laptop.

jihoonson · 2015-04-12T23:18:56Z

Thanks. I think that this work is almost done with the latest patch.
I'll commit tonight after some tests.

dongjoon-hyun · 2015-04-13T00:09:59Z

Thank you, @jihoonson .

jihoonson · 2015-04-13T13:34:36Z

+1 LGTM!
Thanks for your contribution.
I think that it would be great if there are any test cases, but it can be added at https://issues.apache.org/jira/browse/TAJO-1435.
I'll commit shortly.

jihoonson reviewed Apr 5, 2015
View reviewed changes

dongjoon-hyun changed the title ~~TAJO-1430: Implement Query Parsing Result Caching~~ TAJO-1430: Improve SQLAnalyzer by session-based parsing-result caching Apr 5, 2015

jihoonson reviewed Apr 6, 2015
View reviewed changes

jihoonson reviewed Apr 12, 2015
View reviewed changes

TAJO-1430: Improve SQLAnalyzer by session-based parsing-result caching

c850d08

asfgit closed this in 7d72088 Apr 13, 2015

dongjoon-hyun deleted the TAJO-1430 branch April 15, 2015 09:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TAJO-1430: Improve SQLAnalyzer by session-based parsing-result caching #442

TAJO-1430: Improve SQLAnalyzer by session-based parsing-result caching #442

dongjoon-hyun commented Mar 20, 2015

dongjoon-hyun commented Mar 24, 2015

dongjoon-hyun commented Mar 25, 2015

dongjoon-hyun commented Apr 4, 2015

dongjoon-hyun commented Apr 5, 2015

jihoonson Apr 5, 2015

dongjoon-hyun Apr 5, 2015

jihoonson Apr 6, 2015

jihoonson commented Apr 5, 2015

dongjoon-hyun commented Apr 5, 2015

dongjoon-hyun commented Apr 6, 2015

jihoonson Apr 6, 2015

dongjoon-hyun Apr 6, 2015

jihoonson commented Apr 6, 2015

dongjoon-hyun commented Apr 6, 2015

jihoonson commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

jihoonson commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

jihoonson Apr 12, 2015

dongjoon-hyun Apr 12, 2015

jihoonson Apr 12, 2015

dongjoon-hyun Apr 12, 2015

jihoonson Apr 12, 2015

dongjoon-hyun Apr 12, 2015

jihoonson Apr 12, 2015

dongjoon-hyun commented Apr 12, 2015

jihoonson commented Apr 12, 2015

dongjoon-hyun commented Apr 13, 2015

jihoonson commented Apr 13, 2015

		@@ -155,7 +174,7 @@ public SubmitQueryResponse executeQuery(Session session, String query, boolean i
		if (isJson) {
		planningContext = buildExpressionFromJson(query);

TAJO-1430: Improve SQLAnalyzer by session-based parsing-result caching #442

TAJO-1430: Improve SQLAnalyzer by session-based parsing-result caching #442

Conversation

dongjoon-hyun commented Mar 20, 2015

dongjoon-hyun commented Mar 24, 2015

dongjoon-hyun commented Mar 25, 2015

dongjoon-hyun commented Apr 4, 2015

dongjoon-hyun commented Apr 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonson commented Apr 5, 2015

dongjoon-hyun commented Apr 5, 2015

dongjoon-hyun commented Apr 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonson commented Apr 6, 2015

dongjoon-hyun commented Apr 6, 2015

jihoonson commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

jihoonson commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

dongjoon-hyun commented Apr 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Apr 12, 2015

jihoonson commented Apr 12, 2015

dongjoon-hyun commented Apr 13, 2015

jihoonson commented Apr 13, 2015