[SPARK-27105][SQL] Optimize away exponential complexity in ORC predicate conversion

[IvanVergiliev]

What changes were proposed in this pull request?

OrcFilters.createBuilder has exponential complexity in the height of the filter tree due to the way the check-and-build pattern is implemented. We've hit this in production by passing a Column filter to Spark directly, with a job taking multiple hours for a simple set of ~30 filters. This PR changes the checking logic so that the conversion has linear complexity in the size of the tree instead of exponential in its height.

Right now, due to the way ORC SearchArgument works, the code is forced to do two separate phases when converting a given Spark filter to an ORC filter:

1. Check if the filter is convertible.
2. Only if the check in 1. succeeds, perform the actual conversion into the resulting ORC filter.

However, there's one detail which is the culprit in the exponential complexity: phases 1. and 2. are both done using the exact same method. The resulting exponential complexity is easiest to see in the NOT case - consider the following code:

val f1 = col("id") === lit(5)
val f2 = !f1
val f3 = !f2
val f4 = !f3
val f5 = !f4

Now, when we run createBuilder on f5, we get the following behaviour:

1. call createBuilder(f4) to check if the child f4 is convertible
2. call createBuilder(f4) to actually convert it

This seems fine when looking at a single level, but what actually ends up happening is:

• createBuilder(f3) will then recursively be called 4 times - 2 times in step 1., and two times in step 2.
• createBuilder(f2) will be called 8 times - 4 times in each top-level step, 2 times in each sub-step.
• createBuilder(f1) will be called 16 times.

As a result, having a tree of height > 30 leads to billions of calls to createBuilder, heap allocations, and so on and can take multiple hours.

The way this PR solves this problem is by separating the check and convert functionalities into separate functions. This way, the call to createBuilder on f5 above would look like this:

1. call isConvertible(f4) to check if the child f4 is convertible - amortized constant complexity
2. call createBuilder(f4) to actually convert it - linear complexity in the size of the subtree.

This way, we get an overall complexity that's linear in the size of the filter tree, allowing us to convert tree with 10s of thousands of nodes in milliseconds.

The reason this split (check and build) is possible is that the checking never actually depends on the actual building of the filter. The check part of createBuilder depends mainly on:

• isSearchableType for leaf nodes, and
• check-ing the child filters for composite nodes like NOT, AND and OR.
  Situations like the SearchArgumentBuilder throwing an exception while building the resulting ORC filter are not handled right now - they just get thrown out of the class, and this change preserves this behaviour.

This PR extracts this part of the code to a separate class which allows the conversion to make very efficient checks to confirm that a given child is convertible before actually converting it.

Results:
Before:

• converting a skewed tree with a height of ~35 took about 6-7 hours.
• converting a skewed tree with hundreds or thousands of nodes would be completely impossible.

Now:

• filtering against a skewed tree with a height of 1500 in the benchmark suite finishes in less than 10 seconds.

Steps to reproduce

val schema = StructType.fromDDL("col INT")
(20 to 30).foreach { width =>
  val whereFilter = (1 to width).map(i => EqualTo("col", i)).reduceLeft(Or)
  val start = System.currentTimeMillis()
  OrcFilters.createFilter(schema, Seq(whereFilter))
  println(s"With $width filters, conversion takes ${System.currentTimeMillis() - start} ms")
}

Before this PR

With 20 filters, conversion takes 363 ms
With 21 filters, conversion takes 496 ms
With 22 filters, conversion takes 939 ms
With 23 filters, conversion takes 1871 ms
With 24 filters, conversion takes 3756 ms
With 25 filters, conversion takes 7452 ms
With 26 filters, conversion takes 14978 ms
With 27 filters, conversion takes 30519 ms
With 28 filters, conversion takes 60361 ms // 1 minute
With 29 filters, conversion takes 126575 ms // 2 minutes 6 seconds
With 30 filters, conversion takes 257369 ms // 4 minutes 17 seconds

After this PR

With 20 filters, conversion takes 12 ms
With 21 filters, conversion takes 0 ms
With 22 filters, conversion takes 1 ms
With 23 filters, conversion takes 0 ms
With 24 filters, conversion takes 1 ms
With 25 filters, conversion takes 1 ms
With 26 filters, conversion takes 0 ms
With 27 filters, conversion takes 1 ms
With 28 filters, conversion takes 0 ms
With 29 filters, conversion takes 1 ms
With 30 filters, conversion takes 0 ms

How was this patch tested?

There are no changes in behaviour, and the existing tests pass. Added new benchmarks that expose the problematic behaviour and they finish quickly with the changes applied.

[IvanVergiliev]

cc @dongjoon-hyun @cloud-fan - it'd be great if you get a chance to review this! I know it's a bit of a large change, but I tried to make it as isolated as possible and explain it well - let me know if I can be of additional help. (Also, a large part of the diff delta comes from the regenerated benchmark results AFAICT.)

[cloud-fan]

ok to test

[cloud-fan]

can the builder be copied? Then we can simulate the rollback.

[IvanVergiliev]

I don’t see support for copying a builder in the hive source.

[IvanVergiliev]

Just remembered that I should probably apply these changes to the hive module as well. Might be more efficient to do so after we settle on whether and how this is going in so that we don’t have to apply the potential code review changes twice.

[SparkQA]

Test build #103425 has finished for PR 24068 at commit 4eae044.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[IvanVergiliev]

@cloud-fan @dongjoon-hyun ping - let me know if someone else would be better suited to review this, or if there are things I can do to make the review easier!

[cloud-fan]

I checked the implementation of orc filter builder, it creates ExpressionTree internally. ExpressionTree is a public class, can we use it directly at Spark side?

[IvanVergiliev]

ExpressionTree is a public class, but it doesn’t expose any public constructors so we can’t instantiate it without reflection or the like.

The way I understand this is:

• some parts of the hive codebase work with the ExpressionTree directly so the class is public;
• however, the builder performs a bunch of optimizations on the tree, which appears to be the reason one can’t just directly create an ExpressionTree (so that the SearchArgument always has the builder transformations).

[cloud-fan]

Seems like using reflection to instantiate ExpressionTree is easier than this approach.

[IvanVergiliev]

@cloud-fan I pushed a new version that has the same complexity and behaviour but doesn't need the FilterWithConjunctPushdown class. This should help eliminate some of the complexity.

While I agree that building an ExpressionTree directly might result in easier to follow code, I also think it's a much riskier change than this one. A couple of reasons:

1. Without being completely familiar with the Hive codebase, I can't be entirely confident that the transformations performed by the SearchArgumentBuilder are strictly for optimization purposes. There might be other parts of the codebase that depend on the fact that a SearchArgument will have certain properties - which are achieved by creating it from the builder. Using reflection to create the underlying representation directly is obviously not the intended purpose of this code and I'm not confident it will be correct.
2. This PR as it is now doesn't change any external behaviour at all. The resulting ORC SearchArguments will be exactly the same as they were before the change. Using ExpressionTree with reflection instead will be a full rewrite, and especially combined with 1. above, it can generate SearchArguments that are different from the current ones.

So, even if the changes introduced because of reasons 1. and 2. above are both correct, they could still end up producing ORC predicates that are less efficient than the current ones because they haven't been transformed / optimized in the proper way by the builder, and thus introduce performance regressions.

I'm okay taking a stab at the ExpressionTree change, but I'm curious to hear more about which parts of the current solution you find complicated. The way I think about it, this change contains the following at a high level:

1. Split the check and build phases of creating a SearchArgument. Even this change on its own is enough to fix the complexity, taking it from exponential to quadratic. And while it introduces some code duplication, I think it doesn't introduce additional complexity - I actually find it easier to understand the behaviour this way, with the checking and the building separated, and without non-obvious effects resulting in exponential complexity.

2. Memoize the results from check. The results from check don't change, so they don't need to be checked multiple times per node.

After getting rid of the FilterWithConjunctPushdown class in the latest version, this is pretty much all that is left here. We can get rid of the memoization as well, but I don't think that's worth it - it's < 10 lines of obvious code, and it makes the complexity easier to analyze.

Let me know what you think about the newest version and my thoughts above, and thanks for taking a look!

[cloud-fan]

I agree with you that building ExpressionTree directly is not a good idea, but there is another idea I'd like to try.

The problem we have is, the orc filter builder has no way back, so we have to do an extra check before calling the builder methods.

The idea I have is, create a tree-like orc filter representations by our own (similar to ExpressionTree), and convert the Spark filters to this tree representations. After the tree is built, call the real orc filter builder to create the SearchArgument.

[IvanVergiliev]

@cloud-fan interesting! A couple of questions to make sure we're on the same page:

1. If I understand your suggestion correctly, on a high-level it would roughly result in trimming the Spark Filter to only the parts of the tree that are convertible to SearchArguments, and then converting these directly without the need for checking. Is that an accurate representation of your idea, or do you have something else in mind? If it's on the same page, I can think about it some more and take a stab at implementing.

2. What's your optimization function between the different ideas here? Are you thinking about ways to decrease code length, code duplication, complexity / maintainability? Want to make sure we're on the same page about this as well so we can evaluate the ideas similarly.

[cloud-fan]

1. ah yes, that's better! We don't need a new tree-like representation, just reuse the Spark filter tree, with unconvertible part trimmed.

2. The idea is to traverse the tree only twice: one for trimming unconvertible filters, one for building orc filters.

[SparkQA]

Test build #103826 has finished for PR 24068 at commit eb3c158.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[IvanVergiliev]

To clarify with regards to 2. - this is exactly the behavior that this PR introduces. There’s effectively a single pass throughout the tree to check for convertibility, and then a second pass to build the ORC filter. While the build phase calls isConvertible for each node, this doesn't cause additional passes across the tree since the results have already been computed in the initial checking pass.

I’ll think about the details of implementing the trimming and take a stab at an implementation if it seems like it can be made cleaner.

[IvanVergiliev]

@cloud-fan after some noticeable delay, I managed to find the time to implement the tree trimming idea yesterday. I think the result looks pretty nice - thanks for the suggestion!

I ran the relevant parts of the benchmark and the performance appears to be comparable - maybe ~10% slower than the previous version. I imagine this is due to external factors, but since it's such a small difference compared to going from exponential to linear, I don't think it's worth looking into it. I haven't updated the benchmark results in the commit with the most recent code since doing so took a couple hours last time I tried, and the differences are not huge. Let me know if you'd like me to update this.

Looking forward to hearing your thoughts!

[SparkQA]

Test build #105249 has finished for PR 24068 at commit 25b8ce1.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[cloud-fan]

unnecessary change?

[cloud-fan]

what's the difference between Column and Expression in this benchmark?

[IvanVergiliev]

Good question. The initial idea was that the Expression test only runs the Expression -> SearchArgument conversion without any actual Spark jobs. This is useful because once we get away from the exponential complexity and get down to linear vs quadratic, there are a lot of other Spark components that become slower than the predicate conversion. Thus, having an isolated benchmark can be helpful in getting a more detailed read on the performance of this individual component. I'll add a comment explaining that in the code.

Regarding the Column vs Expression part, for some reason when I was researching it, I found it easier to go from an Expression to a SearchArgument. I now see that it's trivial to get the underlying Expression from a Column so I'll just do that for consistency (and it's also a bit more readable as well).

[cloud-fan]

hmm I feel this one is not very useful. We just wrap the trimmer filter with it and then throw it away when building ORC filter. I think we can add assert when building ORC filter, to make sure the passed filter is already trimed.

[IvanVergiliev]

How would you implement such an assert though? I can't think of an easy way to do so without running the convertibility checks again and going down the same rabbit hole we're trying to get out of.

The other benefit is that this makes it kind of obvious at compile time that you can only invoke the createBuilder function with an already trimmed down filter. Of course, you can still get around it by manually wrapping the filter, but at least you got the compiler screaming at you instead of silently accepting it and then failing at runtime.

Another option I considered is calling the function createBuilderForTrimmedFilter, but this felt easier to ignore and still get wrong.

[cloud-fan]

need to update comment?

[IvanVergiliev]

Oh, right - I actually added it back, but apparently put it in the wrong place. Moving.

[cloud-fan]

we can remove the if isSearchableType(dataTypeMap(attribute)) now?

[IvanVergiliev]

Yeah, removed. I figured I'll keep them for safety in case something goes out of date between the two functions, but a lot of other things will likely break anyway if things get out of sync.

Btw if you have thoughts on how to make sure the functions stay in sync beyond the comments I've put now, I'm very open to hearing them.

[cloud-fan]

why change the existing benchmark?

[cloud-fan]

One thing we can do is, add an assert(isSearchableType(dataTypeMap(attribute))) at the beginning.

case _ =>
  assert(isSearchableType(dataTypeMap(attribute)))
  subexpression match {
    case EqualTo(attribute, value) => ...
    case EqualNullSafe(attribute, value) => ...
    ...
    case _ => throw new IllegalStateException(...)
  }

[cloud-fan]

This also guarantees that the input filter is already trimmed, so that we can get rid of the TrimmedFilter.

[IvanVergiliev]

Hm, that's a good idea in principle, but I don't think the outer case-match with the assert can be implemented properly. That is, when I do:

      case _ =>
        assert(isSearchableType(dataTypeMap(attribute)))
        subexpression match {
          case EqualTo(attribute, value) =>
      ...

, the compiler doesn't know what attribute is because the match is simply against _. AFAICT there's no common class that I can match against here to say "match any Filter that has an attribute field" since all the relevant filters are just implemented as separate case classes that just extend Filter directly. The idea of using a structural type to signify "has the field attribute" also crossed my mind, but it seems like you can't match on a structural type.

So AFAICT the options for implementing this are:

• Add the assert to every single case. This seems a bit too verbose.
• Bring back the if isSearchableType(...) on every case, and then throw an exception in the final case _ instead of returning true.
  What are your thoughts? Another question is whether we actually want this to fail if things go wrong. I'm a bit concerned about failing since it could prevent people from using this if something in the implementation gets inconsistent.

The other thing is that all of this will only happen at runtime. The TrimmedFilter is a way to alert the developer at compile time that they shouldn't be calling this method with anything that's not trimmed, whereas these options could fail unexpectedly at runtime.

Let me know what you think about these and we can figure out what to do here.

[cloud-fan]

Add the assert to every single case. This seems a bit too verbose.

I think it's as verbose as adding a if isSearchableType(...) for every case and I'm ok with it.

The TrimmedFilter is a way to alert the developer at compile time that they shouldn't be calling this method with anything that's not trimmed

Personally I think it's just a way to force the developers to look at the doc and see what they should do before calling this method. I don't see a big value of it, as I don't think there will be more callers of this method. It's acceptable to fail some tests and alert the developers to change their code.

[SparkQA]

Test build #105276 has finished for PR 24068 at commit 2119c12.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #105404 has finished for PR 24068 at commit a146b05.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[IvanVergiliev]

@cloud-fan I took a stab at a slightly different approach to structuring the code in https://github.com/IvanVergiliev/spark/pull/2/files . The idea is to implement filtering and building in the same match expression, with an enum that tells us whether to perform a filter or a build operation. This has the following benefits:

• All the logic for a given predicate is grouped logically in the same place. You don't have to scroll across the whole file to see what the filter action for an And is while you're looking at the build action.
• You can't really add a new predicate to the set of filtered predicates without also defining a Build action for it - this fails the exhaustiveness check on ActionType.

The only annoying part is the need for the Either to allow the filter and build actions to return different types. I experimented a bit with using type members on ActionType so that I can define the correct return type for each action and then use that as the return type for performAction, but I don't think this is really expressible in Scala. I don't think this is a big problem.

Overall I'm pretty happy with that version of the code. Let me know if that looks reasonable to you, and if so - I can merge that branch into this one and we can then wrap up the review.

[cloud-fan]

that sounds like a good idea! Can you merge it and fix the conflict? thanks!

[SparkQA]

Test build #105816 has finished for PR 24068 at commit 28feafe.

• This patch fails due to an unknown error code, -9.
• This patch does not merge cleanly.
• This patch adds no public classes.

[IvanVergiliev]

@cloud-fan I applied the changes to the hive/ subtree as well. It seems like it wasn't updated to reflect some of the recent changes to the other files (for example, it doesn't seem to have the quoted attribute name support) - I only applied the minimum amount of changed required to get the same algorithmic behaviour. I haven't applied the quoted attribute name support and other differences.

@gatorsmile I see what you're saying. I got rid of the benchmark that only does the filter conversion step since that indeed seemed a bit out of place with the rest. The remaining new benchmark is Pushdown benchmark with unbalanced Column which tests the full filter-with-pushdown sequence that the rest of the benchmarks cover. This remaining benchmark is almost the same as Pushdown benchmark with many filters which was added in #22313 with the introduction of the buildTree function. I think it makes sense to have these two benchmarks next to each other. Let me know if you'd like me to move both of them to a new benchmark file. (I have a small concern that if we move them to a separate file, it'll be easier to forget to run them when making changes - because, as you say, the process is not fully automated at the moment. Thus, I slightly prefer keeping them where they are, but I'm open to moving them too.)

[IvanVergiliev]

@gatorsmile also added reproduction steps (modelled after #22313 ) to the commit description. 30 filters go from 7 minutes to 300ms 🙂

[SparkQA]

Test build #106240 has finished for PR 24068 at commit 27ed4f9.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[gatorsmile]

@IvanVergiliev Thank you for your contribution! The PR description looks much better!

@IvanVergiliev and @gengliangwang I have not read the implementation details yet. Could you show us the perf difference between two approaches?

[gengliangwang]

@gatorsmile I take a quick check, the perf is quite close. The key difference between the two approaches is how the code is implemented: abstraction and readability.

[IvanVergiliev]

@gatorsmile yep, the performance is pretty much the same because the two implementations are exactly the same at a high level. The two main high-level steps are:

1. Trim the Filter tree so that unconvertible filters are removed and we don't need to check before building ORC filters in the next step.
2. Convert the trimmed filter, knowing that it's fully convertible at this point.

The main differences are in code structure. At a high level, the two main differences are:

1. Whether the trimming and conversion logic are in the same function, or in two separate functions. I actually had this implemented with two separate functions a few commits back - you can see https://github.com/IvanVergiliev/spark/pull/2/files for when I made the change to merge them in the same function. Notice that it's in a separate PR because I realize it's a controversial change. However, as mentioned back then, I think keeping the filtering and building logic in the same method has the following benefits:

• All the logic for a given predicate is grouped logically in the same place. You don't have to scroll across the whole file to see what the filter action for an And is while you're looking at the build action.
• You can't really add a new predicate to the set of filtered predicates without also defining a Build action for it - this fails the exhaustiveness check on ActionType.

@cloud-fan agreed that this does sound like an improvement in #24068 (comment) , so I merged the change into the main PR. I still think this has code design benefits over the separate functions, but I'm open to reverting to the previous version if there's consensus that it's better.

2. Whether we call buildSearchArgument while trimming the tree. While this provides for a shorter implementation, I think it makes the contract of buildSearchArgument very weak. If the trimming and building are independent from each other, we have a clear, well-defined contract for buildSearchArgument that's basically this

We always pass a fully convertible Filter, so buildSearchArgument can just depend on that and return a Builder.

Instead, if we buildSearchArgument in order to trim the filters sometimes as well, we get a way murkier, unclear contract that goes something like:

We mostly pass a convertible Filter, but sometimes we actually pass whatever we get, and depend on buildSearchArgument to figure out if it's convertible or not.

This leads to all kinds of questions:

• if we always trim the Filter first, why do we need to return Option when building?

• if I want to use the buildSearchArgument method and I see that it returns an Option, I might just as well assume that it will handle non-convertible filters just fine. However, this is only partially true - it can handle some of them fine, and can break for others.

• There's no clear single responsibility of the buildSearchArgument method. The name implies that the only responsibility of the method is to build things, but in fact it's also sometimes used for trimming.

  So while this saves some code, I think it makes the overall design and separation of concerns much less clear.

tl;dr I've gone through similar passes as the one proposed by @gengliangwang, but I've had specific design reasons to move away from them.

I'm happy to go back to a version where the filtering and building are written in separate methods if people like this better. I'm more sceptical about the reuse of buildSearchArgument for the reasons I explained above, but I can also make this happen if people think it's a better approach.

This got pretty long, but it also summarizes ~50 commits of changes and the discussion surrounding them. Let me know if anything is unclear or doesn't make sense.

[cloud-fan]

If I read this comment correctly, it seems that we should just remove the next benchmark, as string filter and Column-based filter have no difference regarding performance. Is there any other critical difference that I missed?

[IvanVergiliev]

@cloud-fan the two go through different code paths. The string-based one was added in #22313 , but it doesn't expose the slowness when passing a Column filter directly. That is, the string-based one was fast before this PR. The one this PR fixes is specifically when passing in a Column directly to something like df.filter(Column).

[cloud-fan]

I still can't get it. Both the string filter and column-based filter will become an Expression in the Filter operator. The differences I see are

1. the new benchmark builds a larger filter
2. the new benchmark use Or instead of And.

[dongjoon-hyun]

Sorry for late review, all.

@IvanVergiliev . Could you make a separate benchmark for both ORC v1 OrcFilters.createFilter and ORC v2 OrcFilters.createFilter. We don't need a disk access benchmark for this PR.

[dongjoon-hyun]

Stddev(ms) 623 seems high here although it doesn't matter to the ratio.

[dongjoon-hyun]

The laptop was stable here? Any other jobs?

[cloud-fan]

@IvanVergiliev I'd suggest we revert all the benchmark changes, and we write a simple microbenmark to test OrcFilters, and post the benchmark code and result in PR description.

Currently we do not run benchmarks automatically for Spark, so perf regressions rely on user reports.

[dongjoon-hyun]

+1 for @cloud-fan 's suggestion. Also, I saw that @gatorsmile also gave the same advice before.

[IvanVergiliev]

Thanks for the feedback! Just clarifying before I make any changes.

• @cloud-fan sounds good, I can get rid of the benchmark changes.

and we write a simple microbenmark to test OrcFilters
Would that be different from the benchmark I added to the PR description per @gatorsmile's suggestion? The one in the PR does have disk access, but I think it's pretty easy to understand, and it does an excellent job at showing that the exponential behaviour goes away. Let me know if you have something else in mind.

Could you make a separate benchmark for both ORC v1 OrcFilters.createFilter and ORC v2 OrcFilters.createFilter.

• @dongjoon-hyun it seems this suggestion would no longer be relevant if we get rid of the benchmark changes and only keep the benchmark results in the PR? Let me know if I got this wrong, and what you had in mind if you'd still like me to do this.

[cloud-fan]

@IvanVergiliev I'd suggest something more low-level, e.g.

val filters = ...
val start = System.currentTimeMillis()
OrcFilter.createFilter...
print("time: " + (start - System.currentTimeMillis())) 

[IvanVergiliev]

@cloud-fan I reverted the benchmark changes, and updated the benchmark in the PR description so it only runs the filter conversion. It's even nicer to look at now, with the old version very visibly increasing 2x on each iteration, and the new one taking roughly 0 ms for each size.

I don't remember if we came to a conclusion on whether I should bring this PR back into a state where it has separate filter and build methods so the followup changes are easier, or if we should merge it as is and do all changes in followups. Let me know what you'd like me to do.

There's one minor caveat with benchmark code. The OrcFilters class is marked private[sql], so I needed to do some Scala gymnastics to enable the benchmark to call it directly. This is the full version of the code:

// Paste this into spark-shell using the `-raw` flag so it gets interpreted as
// a Scala file and so that we can trick spark-shell into thinking our class is
// actually in the `sql` package and can thus access `OrcFilters`.
:paste -raw

package org.apache.spark.sql

import org.apache.spark.sql.execution.datasources.orc._
import org.apache.spark.sql.types._
import org.apache.spark.sql.sources._
object OrcFiltersTest {
  def foo(): Unit = {
    val schema = StructType.fromDDL("col INT")
    (20 to 30).foreach { width =>
      val whereFilter = (1 to width).map(i => EqualTo("col", i)).reduceLeft(Or)
      val start = System.currentTimeMillis()
      OrcFilters.createFilter(schema, Seq(whereFilter))
      println(s"With $width filters, conversion takes ${System.currentTimeMillis() - start} ms")
    }
  }
}

[SparkQA]

Test build #106618 has finished for PR 24068 at commit 24dcc24.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[cloud-fan]

@IvanVergiliev thanks for your great work! merging to master

[cloud-fan]

@gengliangwang feel free to open a PR to simplify the code as you proposed.