Skip to content

DRILL-4618: Fix hive function loader not correctly take random flag; … #509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
chunhui-shi wants to merge 1 commit into from
Closed

DRILL-4618: Fix hive function loader not correctly take random flag; … #509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion ...b/storage-hive/core/src/main/java/org/apache/drill/exec/expr/fn/HiveFunctionRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.drill.exec.expr.fn;

import java.util.HashSet;
import java.util.Map;
import java.util.Set;

import org.apache.calcite.rel.type.RelDataType;
Expand Down Expand Up @@ -73,6 +74,25 @@ public HiveFunctionRegistry(DrillConfig config) {
for (Class<? extends UDF> clazz : udfClasses) {
register(clazz, methodsUDF);
}

if (logger.isTraceEnabled()) {
StringBuilder allHiveFunctions = new StringBuilder();
for (Map.Entry<String, Class<? extends GenericUDF>> method : methodsGenericUDF.entries()) {
allHiveFunctions.append(method.toString()).append("\n");
}
logger.trace("Registered Hive GenericUDFs: [\n{}]", allHiveFunctions);

StringBuilder allUDFs = new StringBuilder();
for (Map.Entry<String, Class<? extends UDF>> method : methodsUDF.entries()) {
allUDFs.append(method.toString()).append("\n");
}
logger.trace("Registered Hive UDFs: [\n{}]", allUDFs);
StringBuilder allNonDeterministic = new StringBuilder();
for (Class<?> clz : nonDeterministicUDFs) {
allNonDeterministic.append(clz.toString()).append("\n");
}
logger.trace("Registered Hive nonDeterministicUDFs: [\n{}]", allNonDeterministic);
}
}

@Override
Expand All @@ -96,7 +116,7 @@ private <C,I> void register(Class<? extends I> clazz, ArrayListMultimap<String,C
}

UDFType type = clazz.getAnnotation(UDFType.class);
if (type != null && type.deterministic()) {
if (type != null && !type.deterministic()) {
nonDeterministicUDFs.add(clazz);
}

Expand Down
10 changes: 10 additions & 0 deletions ...ib/storage-hive/core/src/test/java/org/apache/drill/exec/fn/hive/TestInbuiltHiveUDFs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,4 +94,14 @@ public void testIf() throws Exception {
.go();
}

@Test // DRILL-4618
public void testRand() throws Exception {
String query = "select 2*rand()=2*rand() col1 from (values (1))";
testBuilder()
.sqlQuery(query)
.unOrdered()
.baselineColumns("col1")
.baselineValues(false)
.go();
}
}
3 changes: 3 additions & 0 deletions exec/java-exec/src/main/java/org/apache/drill/exec/expr/EqualityVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ public Boolean visitFunctionHolderExpression(FunctionHolderExpression holder, Lo
if (!holder.getName().equals(((FunctionHolderExpression) value).getName())) {
return false;
}
if (holder.isRandom()) {
return false;
}
return checkChildren(holder, value);
}

Expand Down
2 changes: 1 addition & 1 deletion exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,7 @@ public HoldingContainer visitFunctionCall(FunctionCall call, ClassGenerator<?> g
@Override
public HoldingContainer visitFunctionHolderExpression(FunctionHolderExpression holder, ClassGenerator<?> generator) throws RuntimeException {
HoldingContainer hc = getPrevious(holder, generator.getMappingSet());
if (hc == null) {
if (hc == null || holder.isRandom()) {
Copy link
Contributor

@StevenMPhillips StevenMPhillips May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is not sufficient to cover all cases. If random() is just part of the expression, then this won't help. For example, the expression 2 * random().

I think a better way to fix this would be to modify EqualityVisitor.visitFunctionHolderExpression() to return false if the function is random. That way any expression which contains a random function will never be considered equal to another expression.

hc = super.visitFunctionHolderExpression(holder, generator);
put(holder, hc, generator.getMappingSet());
}
Expand Down
11 changes: 11 additions & 0 deletions exec/java-exec/src/test/java/org/apache/drill/TestFunctionsQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,4 +912,15 @@ public void testConcatSingleInput() throws Exception {
.baselineValues("foo")
.go();
}

@Test
public void testRandom() throws Exception {
String query = "select 2*random()=2*random() as col1 from (values (1))";
testBuilder()
.sqlQuery(query)
.unOrdered()
.baselineColumns("col1")
.baselineValues(false)
.go();
}
}