Steve Carlin has posted comments on this change. (
http://gerrit.cloudera.org:8080/17093 )
Change subject: IMPALA-10525: Add param to BuiltinsDb to defer initialization
......................................................................
Patch Set 1:
(3 comments)
For the most part, the BuiltIn instance will have the exact same contents. In
my implementation though, I created a static file containing all the dirty
details, like the C++ signatures. After the Builtin instance is created, the
"BuiltInDb.getInstance().addFunction is called for all the built in functions.
I went with this implementation because I was looking for minimal impact on the
Impala code base. I could go for something a bit more complicated if this
doesn't meet code review requirements.
If we're concerned about immutability, one potential solution is to have a
Loader interface that loads in the BuiltinDb the way an external user of the
class prefers. I think it would have to be a singleton as well and look like
this:
private static volatile Loader loader;
public static setLoader(Loader loader) {
this.loader = loader;
}
...and then in the Builtins constructor, call loader.initBuiltins(this);
The Impala version would have a BuiltinLoader class with the "initBuiltins()"
method that contains all the static initializations.
Would that be more to your liking?
http://gerrit.cloudera.org:8080/#/c/17093/1/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
File fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java:
http://gerrit.cloudera.org:8080/#/c/17093/1/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@81
PS1, Line 81: public static synchronized Db getInstance(boolean initBuiltins)
{
> Should we make 'initBuiltins' a static field so if someone call both getIns
See general comment for perhaps a better way to implement.
But if I stick with this, the thing is that in my implementation, it's really
only slightly deferred. The parameter only matters when the singleton hasn't
been initialized. After that, it doesn't matter if you call it with true or
false.
http://gerrit.cloudera.org:8080/#/c/17093/1/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@91
PS1, Line 91: if (initBuiltins) {
: initBuiltins();
: }
> This seems disable the init of all builtins. My concern is the following:
See general comment for perhaps a better way to implement.
But to address these comments:
The third party tool is responsible for all the initializations. There's no
mix. And the 3rd party should match the functions that Impala uses to populate.
There are future plans to make sure that the Impala initialization matches the
3rd party initialization, but that is a bigger rewrite, and beyond the scope of
what can be developed at this point.
http://gerrit.cloudera.org:8080/#/c/17093/1/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@99
PS1, Line 99: private
> initBuiltins() is private. Will it be called after we init the singleton wi
See general comment for perhaps a better way to implement.
But to address this comment:
The third part is using BuiltinDb.addFunction() to populate the functions.
I can address the rename, will do so after discussing the other proposal I
mentioned on the top level.
--
To view, visit http://gerrit.cloudera.org:8080/17093
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1941a2494efe15b63514d849873eeb1c4ed8a981
Gerrit-Change-Number: 17093
Gerrit-PatchSet: 1
Gerrit-Owner: Steve Carlin <scar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com>
Gerrit-Reviewer: Steve Carlin <scar...@cloudera.com>
Gerrit-Comment-Date: Mon, 22 Feb 2021 17:08:36 +0000
Gerrit-HasComments: Yes
