You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2020/03/31 09:51:56 UTC
[GitHub] [drill] nielsbasjes opened a new pull request #2044: DRILL-7678:
Update Yauaa dependency
nielsbasjes opened a new pull request #2044: DRILL-7678: Update Yauaa dependency
URL: https://github.com/apache/drill/pull/2044
# [DRILL-7678](https://issues.apache.org/jira/browse/DRILL-7678): Update Yauaa dependency
## Description
This is to update the version of the useragent analyzer to the latest released version.
Changes:
- Update to latest version of Yauaa
- Extracted the instantiation of the analyzer into a separate class because the dynamic code compiler could not handle the way the classes and builders work together (see https://github.com/nielsbasjes/yauaa/issues/204 reported by @cgivre )
- Only 1 instance of the analyzer (saves a lot of memory)
- Now use the caching also present in the analyzer (i.e. do not use the 'Direct' version)
- Cache the 'allFields' as this has a direct performance impact.
- Fix a java warning around generics in the test class.
## Documentation
No changes.
## Testing
All existing tests were sufficient.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401566246
##########
File path: contrib/udfs/src/test/java/org/apache/drill/exec/udfs/TestUserAgentFunctions.java
##########
@@ -102,7 +102,7 @@ public void testEmptyFieldName() throws Exception {
@Test
public void testNullUserAgent() throws Exception {
String query = "SELECT parse_user_agent(CAST(null as VARCHAR)) AS agent FROM (values(1))";
- Map emptyMap = new HashMap();
+ Map<?, ?> emptyMap = new HashMap<>();
Review comment:
```suggestion
Map<?, ?> emptyMap = Collections.emptyMap();
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r400898546
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
Review comment:
Thanks!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401235092
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
it's better to use here DCL to avoid constant synchronization.
@vvysotskyi @paul-rogers it is ok to call such methods from UDFs, I recall there was some discussions in Jira about scalar replacement. Does it apply here?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401594804
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
Just to clarify you are not going to implement holder on demand?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on issue #2044: DRILL-7678:
Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on issue #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#issuecomment-609408492
+1, LGTM.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] nielsbasjes commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401245200
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
What do you mean with DCL?
I happy to change the code if needed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401835648
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentFunctions.java
##########
@@ -46,11 +46,14 @@
DrillBuf outBuffer;
@Workspace
- nl.basjes.parse.useragent.UserAgentAnalyzerDirect uaa;
+ nl.basjes.parse.useragent.UserAgentAnalyzer uaa;
+
+ @Workspace
+ java.util.List<String> allFields;
public void setup() {
- uaa = nl.basjes.parse.useragent.UserAgentAnalyzerDirect.newBuilder().dropTests().hideMatcherLoadStats().build();
- uaa.getAllPossibleFieldNamesSorted();
+ uaa = org.apache.drill.exec.udfs.UserAgentAnalyzerProvider.getInstance();
+ allFields = uaa.getAllPossibleFieldNamesSorted();
Review comment:
Thinking this through... Drill is multi-threaded. We can have, say, 20 threads all running the same query, evaluating the same expression. Having the list here seems thread-safe. I wonder, however, is the list of fields essentially static? Seems to be, there are no parameters passed to the instance. If so, does it make sense to store the field list in the provider so we don't have to build it for every thread of every query?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #2044:
DRILL-7678: Update Yauaa dependency
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2044: DRILL-7678: Update Yauaa dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r400885680
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
Review comment:
Nit: Please indent with 2 spaces rather than 4.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401835854
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentFunctions.java
##########
@@ -92,11 +95,14 @@ public void eval() {
DrillBuf outBuffer;
@Workspace
- nl.basjes.parse.useragent.UserAgentAnalyzerDirect uaa;
+ nl.basjes.parse.useragent.UserAgentAnalyzer uaa;
+
+ @Workspace
+ java.util.List<String> allFields;
public void setup() {
- uaa = nl.basjes.parse.useragent.UserAgentAnalyzerDirect.newBuilder().dropTests().hideMatcherLoadStats().build();
- uaa.getAllPossibleFieldNamesSorted();
+ uaa = org.apache.drill.exec.udfs.UserAgentAnalyzerProvider.getInstance();
+ allFields = uaa.getAllPossibleFieldNamesSorted();
Review comment:
Same suggestion
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r402229769
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentAnalyzerProvider.java
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentAnalyzerProvider {
+
+ public static UserAgentAnalyzer getInstance() {
+ return UserAgentAnalyzerHolder.INSTANCE;
+ }
+
+ private static class UserAgentAnalyzerHolder {
+ private static final UserAgentAnalyzer INSTANCE = UserAgentAnalyzer.newBuilder()
Review comment:
@paul-rogers here is some explanation that you might find useful: https://en.m.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on issue #2044: DRILL-7678: Update Yauaa
Dependency
Posted by GitBox <gi...@apache.org>.
cgivre commented on issue #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#issuecomment-606616190
@nielsbasjes
Thanks for the changes. LGTM +1
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401944282
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentAnalyzerProvider.java
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentAnalyzerProvider {
+
+ public static UserAgentAnalyzer getInstance() {
+ return UserAgentAnalyzerHolder.INSTANCE;
+ }
+
+ private static class UserAgentAnalyzerHolder {
+ private static final UserAgentAnalyzer INSTANCE = UserAgentAnalyzer.newBuilder()
Review comment:
@arina-ielchiieva, thanks for the explanation. I understand the reasoning. However, what I see, at least in the debugger, is that statics are not initialized until the first use of the class. A reference is not a use, only a call is.
However, this behavior may be unique to the debugger, perhaps the non-debug execution works differently. Still, easy enough to test so we can know for certain. Would be good to nail down the exact behavior so we don't add excess complexity unnecessarily.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] nielsbasjes commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401465669
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
I know Double checked locking, I was just confused about 'DCL'.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401551826
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
@vvysotskyi thanks for confirmation.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401675033
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
Yes, it would be really good to use holder on demand implementation because initialization-on-demand holder is always best practice for implementing singleton pattern.
Regarding how to implement it, example how to implement it was present in the link I have posted previously: `3.2. Initialization on Demand`.
To make things quicker, I have already adopted it to your use case, please replace your class `UserAgentUtils` with new class `UserAgentAnalyzerProvider` and call `UserAgentAnalyzerProvider.getInstance()` in your UDFs code.
```
public class UserAgentAnalyzerProvider {
public static UserAgentAnalyzer getInstance() {
return UserAgentAnalyzerHolder.INSTANCE;
}
private static class UserAgentAnalyzerHolder {
private static final UserAgentAnalyzer INSTANCE = UserAgentAnalyzer.newBuilder()
.dropTests()
.hideMatcherLoadStats()
.immediateInitialization()
.build();
}
}
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401546893
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
No problem, it would be nice if you'll use here holder on demand, looks like it suits here perfectly.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] nielsbasjes commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401660833
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
@arina-ielchiieva 2 things:
1. If you want it using a "holder on demand" then I'll implement it. This is your project, I'm just passing by to help on this single aspect. Your call.
2. My knowledge of the inner workings of Drill is too limited to understand how you want it. I have no clue what a "holder on demand" looks like. Sorry.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] nielsbasjes commented on a change in pull request #2044:
DRILL-7678: Update Yauaa dependency
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on a change in pull request #2044: DRILL-7678: Update Yauaa dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r400897212
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
Review comment:
Done
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401833874
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentAnalyzerProvider.java
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentAnalyzerProvider {
+
+ public static UserAgentAnalyzer getInstance() {
+ return UserAgentAnalyzerHolder.INSTANCE;
+ }
+
+ private static class UserAgentAnalyzerHolder {
+ private static final UserAgentAnalyzer INSTANCE = UserAgentAnalyzer.newBuilder()
Review comment:
Jumping in a bit late here. Isn't one layer of class sufficient to hold the static instance?
```
public class UserAgentAnalyzerProvider {
private static final INSTANCE = UserAgentAnalyzer.newBuilder()
.dropTests()
.hideMatcherLoadStats()
.immediateInitialization()
.build();
public static UserAgentAnalyzer getInstance() {
return INSTANCE;
}
}
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] asfgit closed pull request #2044: DRILL-7678: Update Yauaa
Dependency
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] nielsbasjes commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401575648
##########
File path: contrib/udfs/src/test/java/org/apache/drill/exec/udfs/TestUserAgentFunctions.java
##########
@@ -102,7 +102,7 @@ public void testEmptyFieldName() throws Exception {
@Test
public void testNullUserAgent() throws Exception {
String query = "SELECT parse_user_agent(CAST(null as VARCHAR)) AS agent FROM (values(1))";
- Map emptyMap = new HashMap();
+ Map<?, ?> emptyMap = new HashMap<>();
Review comment:
Fixed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] nielsbasjes commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401581438
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
Note that this is called only during the `setup()` so I expect the number of calls to this to be very small and as such not a performance problem.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] nielsbasjes commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r402179337
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentAnalyzerProvider.java
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentAnalyzerProvider {
+
+ public static UserAgentAnalyzer getInstance() {
+ return UserAgentAnalyzerHolder.INSTANCE;
+ }
+
+ private static class UserAgentAnalyzerHolder {
+ private static final UserAgentAnalyzer INSTANCE = UserAgentAnalyzer.newBuilder()
+ .dropTests()
+ .hideMatcherLoadStats()
+ .immediateInitialization()
+ .build();
+ }
+}
Review comment:
Fixed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401926555
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentAnalyzerProvider.java
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentAnalyzerProvider {
+
+ public static UserAgentAnalyzer getInstance() {
+ return UserAgentAnalyzerHolder.INSTANCE;
+ }
+
+ private static class UserAgentAnalyzerHolder {
+ private static final UserAgentAnalyzer INSTANCE = UserAgentAnalyzer.newBuilder()
Review comment:
@paul-rogers may I answer this question.
Your suggestion will also work but will contradict with holder on demand singleton approach, i.e. it's not lazy init approach. Your example will init singleton instance when JVM starts while having inner class will init singleton only during first call of `getInstance` method.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r403694906
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentAnalyzerProvider.java
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentAnalyzerProvider {
+
+ public static UserAgentAnalyzer getInstance() {
+ return UserAgentAnalyzerHolder.INSTANCE;
+ }
+
+ private static class UserAgentAnalyzerHolder {
+ private static final UserAgentAnalyzer INSTANCE = UserAgentAnalyzer.newBuilder()
Review comment:
@paul-rogers totally agree with you, since there is not other static stuff there class won't be loaded prematurely, unless with time something static is added there. Anyway, having inner class does not give much complexity and since you ok, we can leave it as it to comply with pattern code template.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r402067486
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentAnalyzerProvider.java
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentAnalyzerProvider {
+
+ public static UserAgentAnalyzer getInstance() {
+ return UserAgentAnalyzerHolder.INSTANCE;
+ }
+
+ private static class UserAgentAnalyzerHolder {
+ private static final UserAgentAnalyzer INSTANCE = UserAgentAnalyzer.newBuilder()
Review comment:
Did a little experiment to see if I was confused. Looks like a single layer of provider will work fine.
Two classes:
```
public class DummyProvider {
private static int instance = 10;
static {
System.out.println("DummyProvider loaded");
}
public static int instance() { return instance; }
}
```
And:
```
public class DummyClient {
public static void main(String[] args) {
System.out.println("Main starting");
System.out.println("Getting instance");
System.out.println(DummyProvider.instance());
System.out.println("Exiting");
}
}
```
Output:
```
Main starting
Getting instance
DummyProvider loaded
10
Exiting
```
Ran in both the debugger and normally. Both produced the same output.
This suggests that, indeed, the class is not loaded, and statics initialized, until first use (call).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] nielsbasjes commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401791768
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
Thanks! I've updated the code, works on my machine and pushed it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401833986
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentAnalyzerProvider.java
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentAnalyzerProvider {
+
+ public static UserAgentAnalyzer getInstance() {
+ return UserAgentAnalyzerHolder.INSTANCE;
+ }
+
+ private static class UserAgentAnalyzerHolder {
+ private static final UserAgentAnalyzer INSTANCE = UserAgentAnalyzer.newBuilder()
+ .dropTests()
+ .hideMatcherLoadStats()
+ .immediateInitialization()
+ .build();
+ }
+}
Review comment:
Nit: newline
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] nielsbasjes commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
nielsbasjes commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r402180380
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentFunctions.java
##########
@@ -46,11 +46,14 @@
DrillBuf outBuffer;
@Workspace
- nl.basjes.parse.useragent.UserAgentAnalyzerDirect uaa;
+ nl.basjes.parse.useragent.UserAgentAnalyzer uaa;
+
+ @Workspace
+ java.util.List<String> allFields;
public void setup() {
- uaa = nl.basjes.parse.useragent.UserAgentAnalyzerDirect.newBuilder().dropTests().hideMatcherLoadStats().build();
- uaa.getAllPossibleFieldNamesSorted();
+ uaa = org.apache.drill.exec.udfs.UserAgentAnalyzerProvider.getInstance();
+ allFields = uaa.getAllPossibleFieldNamesSorted();
Review comment:
The list of field names is essentially static.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401235092
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
It's better to use here DCL to avoid constant synchronization.
@vvysotskyi @paul-rogers it is ok to call such methods from UDFs? I recall there was some discussions in Jira about scalar replacement. Does it apply here?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401235092
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
It's better to use here DCL to avoid constant synchronization.
@vvysotskyi @paul-rogers it is ok to call such methods from UDFs, I recall there was some discussions in Jira about scalar replacement. Does it apply here?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401551396
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
@arina-ielchiieva, in this case, it is safe to call such methods from UDFs. Problem with scalar replacement occurs when holder is passed to the method.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #2044:
DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r402485830
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentAnalyzerProvider.java
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentAnalyzerProvider {
+
+ public static UserAgentAnalyzer getInstance() {
+ return UserAgentAnalyzerHolder.INSTANCE;
+ }
+
+ private static class UserAgentAnalyzerHolder {
+ private static final UserAgentAnalyzer INSTANCE = UserAgentAnalyzer.newBuilder()
Review comment:
@arina-ielchiieva, thanks; that article is accurate in the case that class `Something` is loaded earlier than the first use of `getInstance()`. Maybe there are other static functions or constants that force earlier class loading.
In this specific case, the only method is `getInstance()` and so the outer class won't be loaded until that time. This means that the inner and outer classes are load at essentially the same time: on that first call to `getInstance()`.
We can commit his PR with the code as it is; it works fine. My concern, however, is that we use static instances all over, and we've kind of relied on the behavior I've been describing. To be consistent, we'd want to go and change all the other uses as well. But, doing so would be unnecessary work in cases, like this one, where a single layer works fine.
Let's go ahead and commit this, then we can continue the discussion without slowing this PR.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2044: DRILL-7678: Update Yauaa Dependency
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2044: DRILL-7678: Update Yauaa Dependency
URL: https://github.com/apache/drill/pull/2044#discussion_r401435441
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/UserAgentUtils.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.drill.exec.udfs;
+
+import nl.basjes.parse.useragent.UserAgentAnalyzer;
+
+public class UserAgentUtils {
+
+ private UserAgentUtils(){}
+
+ private static UserAgentAnalyzer instance = null;
+
+ public static synchronized UserAgentAnalyzer getInstance() {
Review comment:
Double-checked locking but even better to use holder on demand.
Maybe this article can help - https://www.baeldung.com/java-singleton-double-checked-locking
----------------------------------------------------------------
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
With regards,
Apache Git Services