Skip to content

Add block/position convention to all distinct from operators #11983

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

Merged
haozhun merged 1 commit into from
Dec 17, 2018
Merged

Add block/position convention to all distinct from operators #11983

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
21 changes: 21 additions & 0 deletions presto-main/src/main/java/com/facebook/presto/metadata/FunctionRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,29 +490,48 @@ public FunctionRegistry(TypeManager typeManager, BlockEncodingSerde blockEncodin
.scalars(JsonFunctions.class)
.scalars(ColorFunctions.class)
.scalars(ColorOperators.class)
.scalar(ColorOperators.ColorDistinctFromOperator.class)
.scalars(HyperLogLogFunctions.class)
.scalars(QuantileDigestFunctions.class)
.scalars(UnknownOperators.class)
.scalar(UnknownOperators.UnknownDistinctFromOperator.class)
.scalars(BooleanOperators.class)
.scalar(BooleanOperators.BooleanDistinctFromOperator.class)
.scalars(BigintOperators.class)
.scalar(BigintOperators.BigintDistinctFromOperator.class)
.scalars(IntegerOperators.class)
.scalar(IntegerOperators.IntegerDistinctFromOperator.class)
.scalars(SmallintOperators.class)
.scalar(SmallintOperators.SmallintDistinctFromOperator.class)
.scalars(TinyintOperators.class)
.scalar(TinyintOperators.TinyintDistinctFromOperator.class)
.scalars(DoubleOperators.class)
.scalar(DoubleOperators.DoubleDistinctFromOperator.class)
.scalars(RealOperators.class)
.scalar(RealOperators.RealDistinctFromOperator.class)
.scalars(VarcharOperators.class)
.scalar(VarcharOperators.VarcharDistinctFromOperator.class)
.scalars(VarbinaryOperators.class)
.scalar(VarbinaryOperators.VarbinaryDistinctFromOperator.class)
.scalars(DateOperators.class)
.scalar(DateOperators.DateDistinctFromOperator.class)
.scalars(TimeOperators.class)
.scalar(TimeOperators.TimeDistinctFromOperator.class)
.scalars(TimestampOperators.class)
.scalar(TimestampOperators.TimestampDistinctFromOperator.class)
.scalars(IntervalDayTimeOperators.class)
.scalar(IntervalDayTimeOperators.IntervalDayTimeDistinctFromOperator.class)
.scalars(IntervalYearMonthOperators.class)
.scalar(IntervalYearMonthOperators.IntervalYearMonthDistinctFromOperator.class)
.scalars(TimeWithTimeZoneOperators.class)
.scalar(TimeWithTimeZoneOperators.TimeWithTimeZoneDistinctFromOperator.class)
.scalars(TimestampWithTimeZoneOperators.class)
.scalar(TimestampWithTimeZoneOperators.TimestampWithTimeZoneDistinctFromOperator.class)
.scalars(DateTimeOperators.class)
.scalars(HyperLogLogOperators.class)
.scalars(QuantileDigestOperators.class)
.scalars(IpAddressOperators.class)
.scalar(IpAddressOperators.IpAddressDistinctFromOperator.class)
.scalars(LikeFunctions.class)
.scalars(ArrayFunctions.class)
.scalars(HmacFunctions.class)
Expand All @@ -523,10 +542,12 @@ public FunctionRegistry(TypeManager typeManager, BlockEncodingSerde blockEncodin
.scalar(ArrayPositionFunction.class)
.scalars(CombineHashFunction.class)
.scalars(JsonOperators.class)
.scalar(JsonOperators.JsonDistinctFromOperator.class)
.scalars(FailureFunction.class)
.scalars(JoniRegexpCasts.class)
.scalars(CharacterStringCasts.class)
.scalars(CharOperators.class)
.scalar(CharOperators.CharDistinctFromOperator.class)
.scalar(DecimalOperators.Negation.class)
.scalar(DecimalOperators.HashCode.class)
.scalar(DecimalOperators.Indeterminate.class)
Expand Down
53 changes: 33 additions & 20 deletions presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayDistinctFromOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
*/

import com.facebook.presto.spi.block.Block;
import com.facebook.presto.spi.function.BlockIndex;
import com.facebook.presto.spi.function.BlockPosition;
import com.facebook.presto.spi.function.Convention;
import com.facebook.presto.spi.function.IsNull;
import com.facebook.presto.spi.function.OperatorDependency;
Expand All @@ -25,12 +27,10 @@

import java.lang.invoke.MethodHandle;

import static com.facebook.presto.spi.function.InvocationConvention.InvocationArgumentConvention.NULL_FLAG;
import static com.facebook.presto.spi.function.InvocationConvention.InvocationArgumentConvention.BLOCK_POSITION;
import static com.facebook.presto.spi.function.InvocationConvention.InvocationReturnConvention.FAIL_ON_NULL;
import static com.facebook.presto.spi.function.OperatorType.IS_DISTINCT_FROM;
import static com.facebook.presto.spi.type.TypeUtils.readNativeValue;
import static com.facebook.presto.util.Failures.internalError;
import static com.google.common.base.Defaults.defaultValue;

@ScalarOperator(IS_DISTINCT_FROM)
public final class ArrayDistinctFromOperator
Expand All @@ -44,8 +44,8 @@ public static boolean isDistinctFrom(
operator = IS_DISTINCT_FROM,
returnType = StandardTypes.BOOLEAN,
argumentTypes = {"E", "E"},
convention = @Convention(arguments = {NULL_FLAG, NULL_FLAG}, result = FAIL_ON_NULL)) MethodHandle function,
@TypeParameter("E") Type type,
convention = @Convention(arguments = {BLOCK_POSITION, BLOCK_POSITION}, result = FAIL_ON_NULL)) MethodHandle function,
Copy link
Copy Markdown
Contributor

@findepi findepi Dec 5, 2018

Choose a reason for hiding this comment

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

can be at the same time rename function to eg elementIsDistinct ?

@TypeParameter("array(E)") Type type,
@SqlType("array(E)") Block left,
@IsNull boolean leftNull,
@SqlType("array(E)") Block right,
Expand All @@ -61,22 +61,12 @@ public static boolean isDistinctFrom(
return true;
}
for (int i = 0; i < left.getPositionCount(); i++) {
Object leftValue = readNativeValue(type, left, i);
boolean leftValueNull = leftValue == null;
if (leftValueNull) {
leftValue = defaultValue(type.getJavaType());
}
Object rightValue = readNativeValue(type, right, i);
boolean rightValueNull = rightValue == null;
if (rightValueNull) {
rightValue = defaultValue(type.getJavaType());
}
try {
if ((boolean) function.invoke(
leftValue,
leftValueNull,
rightValue,
rightValueNull)) {
if ((boolean) function.invokeExact(
left,
Copy link
Copy Markdown
Contributor

@wenleix wenleix Dec 5, 2018

Choose a reason for hiding this comment

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

Is it possible to use invokeExact here?

Also, I think we can squash all parameters into one line now.

i,
right,
i)) {
return true;
}
}
Expand All @@ -86,4 +76,27 @@ public static boolean isDistinctFrom(
}
return false;
}

@TypeParameter("E")
@SqlType(StandardTypes.BOOLEAN)
public static boolean isDistinctFrom(
@OperatorDependency(
operator = IS_DISTINCT_FROM,
returnType = StandardTypes.BOOLEAN,
argumentTypes = {"E", "E"},
convention = @Convention(arguments = {BLOCK_POSITION, BLOCK_POSITION}, result = FAIL_ON_NULL)) MethodHandle elementIsDistinctFrom,
@TypeParameter("array(E)") Type type,
@BlockPosition @SqlType(value = "array(E)", nativeContainerType = Block.class) Block left,
@BlockIndex int leftPosition,
@BlockPosition @SqlType(value = "array(E)", nativeContainerType = Block.class) Block right,
@BlockIndex int rightPosition)
{
return isDistinctFrom(
elementIsDistinctFrom,
type,
(Block) type.getObject(left, leftPosition),
left.isNull(leftPosition),
(Block) type.getObject(right, rightPosition),
right.isNull(rightPosition));
}
}
41 changes: 34 additions & 7 deletions presto-main/src/main/java/com/facebook/presto/operator/scalar/JsonOperators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@

import com.facebook.presto.spi.ConnectorSession;
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.block.Block;
import com.facebook.presto.spi.function.BlockIndex;
import com.facebook.presto.spi.function.BlockPosition;
import com.facebook.presto.spi.function.IsNull;
import com.facebook.presto.spi.function.LiteralParameters;
import com.facebook.presto.spi.function.ScalarOperator;
Expand Down Expand Up @@ -391,15 +394,39 @@ public static Boolean notEqual(@SqlType(JSON) Slice leftJson, @SqlType(JSON) Sli
}

@ScalarOperator(IS_DISTINCT_FROM)
@SqlType(BOOLEAN)
public static boolean isDistinctFrom(@SqlType(JSON) Slice leftJson, @IsNull boolean leftNull, @SqlType(JSON) Slice rightJson, @IsNull boolean rightNull)
public static class JsonDistinctFromOperator
{
if (leftNull != rightNull) {
return true;
@SqlType(BOOLEAN)
public static boolean isDistinctFrom(@SqlType(JSON) Slice leftJson, @IsNull boolean leftNull, @SqlType(JSON) Slice rightJson, @IsNull boolean rightNull)
{
if (leftNull != rightNull) {
return true;
}
if (leftNull) {
return false;
}
return notEqual(leftJson, rightJson);
Copy link
Copy Markdown
Contributor

@wenleix wenleix Dec 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Not related to this PR itself.

Is the motivation to directly call notEqual here (instead of ask Operator Dependency to provide a MethodHandle) because we want Java to inline the call ?

Copy link
Copy Markdown
Contributor Author

@haozhun haozhun Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The reason notEquals is used here directly is not related to performance. Rather, I believe the direct invocation aligns better with the logic intention this line of code intend to convey. It's also simpler.

}
if (leftNull) {
return false;

@SqlType(BOOLEAN)
public static boolean isDistinctFrom(
@BlockPosition @SqlType(value = JSON, nativeContainerType = Slice.class) Block left,
Copy link
Copy Markdown
Contributor

@findepi findepi Nov 27, 2018

Choose a reason for hiding this comment

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

I know this isn't new annotation, but... What does @BlockPosition mean? This annotates "a block", not "a position" -- this is why i do not understand.

Copy link
Copy Markdown
Contributor

@wenleix wenleix Nov 27, 2018

Choose a reason for hiding this comment

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

@findepi : This parameter is Block, and next parameter is required to be annotated with BlockIndex. This convention is already used in aggregation framework.

Copy link
Copy Markdown
Contributor

@findepi findepi Nov 27, 2018

Choose a reason for hiding this comment

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

So the @BlockPosition annotation means "use block position calling convention"? As it stands in the code, it is hard to understand

if there was no (obvious) name clash, calling it (@Block Block block, @BlockPosition int position) would look better.
or @ContainingBlock Block block, @BlockPosition int position)

This convention is already used ...

i know i am late with my thoughts. I was just a bit surprised by the naming and thought that, maybe, we can improve a bit here.

Copy link
Copy Markdown
Contributor Author

@haozhun haozhun Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@Block wouldn't work, annotation classes and regular classes are in the same namespace.

The annotation can be dropped all together, just like how @IsNull is implemented. Parsing code would be a little more complicated that way, but not at all infeasible. The parsing would require lookahead because the type being Block isn't sufficient indication. But we already do look ahead for @IsNull.

I agree that @BlockPosition is a terrible name. I don't particularly like @ContainingBlock either.

For this PR, I'd leave this as is. This PR isn't introducing the syntax. It's just using it.

Copy link
Copy Markdown
Contributor

@findepi findepi Nov 27, 2018

Choose a reason for hiding this comment

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

@Block wouldn't work, annotation classes and regular classes are in the same namespace.

of course! I was only trying to convey my thinking what the ideal naming could be.

Parsing code would be a little more complicated that way, but not at all infeasible.

Surely my intention isn't to make things more complicated. Maybe we can think of some better name. Or maybe dropping the annotation is the way.

For this PR, I'd leave this as is. This PR isn't introducing the syntax. It's just using it.

sure!

@BlockIndex int leftPosition,
@BlockPosition @SqlType(value = JSON, nativeContainerType = Slice.class) Block right,
@BlockIndex int rightPosition)
{
if (left.isNull(leftPosition) != right.isNull(rightPosition)) {
return true;
}
if (left.isNull(leftPosition)) {
return false;
}
int leftLength = left.getSliceLength(leftPosition);
int rightLength = right.getSliceLength(rightPosition);
if (leftLength != rightLength) {
return true;
}
return !left.equals(leftPosition, 0, right, rightPosition, 0, leftLength);
Copy link
Copy Markdown
Contributor

@wenleix wenleix Dec 5, 2018

Choose a reason for hiding this comment

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

The long term plan is to gradually deprecated Type.equalTo() and use the EQUALS operator with BLOCK_AND_POSITION calling convention right?

What about Type.compareTo() ?

Copy link
Copy Markdown
Contributor

@wenleix wenleix Dec 5, 2018

Choose a reason for hiding this comment

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

Do we want to also implement block position convention for equality, and delegate to use notEquals here? -- We don't need to implement this for now.

}
return notEqual(leftJson, rightJson);
}
}
52 changes: 31 additions & 21 deletions presto-main/src/main/java/com/facebook/presto/operator/scalar/MapDistinctFromOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,23 @@
*/

import com.facebook.presto.spi.block.Block;
import com.facebook.presto.spi.function.BlockIndex;
import com.facebook.presto.spi.function.BlockPosition;
import com.facebook.presto.spi.function.Convention;
import com.facebook.presto.spi.function.IsNull;
import com.facebook.presto.spi.function.OperatorDependency;
import com.facebook.presto.spi.function.ScalarOperator;
import com.facebook.presto.spi.function.SqlType;
import com.facebook.presto.spi.function.TypeParameter;
import com.facebook.presto.spi.type.MapType;
import com.facebook.presto.spi.type.StandardTypes;
import com.facebook.presto.spi.type.Type;

import java.lang.invoke.MethodHandle;

import static com.facebook.presto.spi.function.OperatorType.EQUAL;
import static com.facebook.presto.spi.function.OperatorType.HASH_CODE;
import static com.facebook.presto.spi.function.InvocationConvention.InvocationArgumentConvention.BLOCK_POSITION;
import static com.facebook.presto.spi.function.InvocationConvention.InvocationReturnConvention.FAIL_ON_NULL;
import static com.facebook.presto.spi.function.OperatorType.IS_DISTINCT_FROM;
import static com.facebook.presto.spi.type.TypeUtils.readNativeValue;

@ScalarOperator(IS_DISTINCT_FROM)
public final class MapDistinctFromOperator
Expand All @@ -38,14 +41,9 @@ private MapDistinctFromOperator() {}
@TypeParameter("V")
@SqlType(StandardTypes.BOOLEAN)
public static boolean isDistinctFrom(
@OperatorDependency(operator = EQUAL, returnType = StandardTypes.BOOLEAN, argumentTypes = {"K", "K"})
MethodHandle keyEqualsFunction,
@OperatorDependency(operator = HASH_CODE, returnType = StandardTypes.BIGINT, argumentTypes = {"K"})
MethodHandle keyHashcodeFunction,
@OperatorDependency(operator = IS_DISTINCT_FROM, returnType = StandardTypes.BOOLEAN, argumentTypes = {"V", "V"})
@OperatorDependency(operator = IS_DISTINCT_FROM, returnType = StandardTypes.BOOLEAN, argumentTypes = {"V", "V"}, convention = @Convention(arguments = {BLOCK_POSITION, BLOCK_POSITION}, result = FAIL_ON_NULL))
MethodHandle valueDistinctFromFunction,
@TypeParameter("K") Type keyType,
@TypeParameter("V") Type valueType,
@TypeParameter("map(K, V)") Type mapType,
Copy link
Copy Markdown
Contributor

@wenleix wenleix Dec 5, 2018

Choose a reason for hiding this comment

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

Looks like we can just keep keyType since we no longer need valueType now.

@SqlType("map(K,V)") Block leftMapBlock,
@IsNull boolean leftMapNull,
@SqlType("map(K,V)") Block rightMapBlock,
Expand All @@ -59,18 +57,30 @@ public static boolean isDistinctFrom(
}
// Note that we compare to NOT distinct here and so negate the result.
return !MapGenericEquality.genericEqual(
keyType,
((MapType) mapType).getKeyType(),
leftMapBlock,
rightMapBlock,
(leftMapIndex, rightMapIndex) -> {
Object leftValue = readNativeValue(valueType, leftMapBlock, leftMapIndex);
Object rightValue = readNativeValue(valueType, rightMapBlock, rightMapIndex);
boolean leftNull = leftValue == null;
boolean rightNull = rightValue == null;
if (leftNull || rightNull) {
return leftNull == rightNull;
}
return !(boolean) valueDistinctFromFunction.invoke(leftValue, leftNull, rightValue, rightNull);
});
(leftMapIndex, rightMapIndex) -> !(boolean) valueDistinctFromFunction.invokeExact(leftMapBlock, leftMapIndex, rightMapBlock, rightMapIndex));
}

@TypeParameter("K")
@TypeParameter("V")
@SqlType(StandardTypes.BOOLEAN)
public static boolean isDistinctFrom(
@OperatorDependency(operator = IS_DISTINCT_FROM, returnType = StandardTypes.BOOLEAN, argumentTypes = {"V", "V"}, convention = @Convention(arguments = {BLOCK_POSITION, BLOCK_POSITION}, result = FAIL_ON_NULL))
MethodHandle valueDistinctFromFunction,
@TypeParameter("map(K, V)") Type mapType,
@BlockPosition @SqlType(value = "map(K,V)", nativeContainerType = Block.class) Block left,
@BlockIndex int leftPosition,
@BlockPosition @SqlType(value = "map(K,V)", nativeContainerType = Block.class) Block right,
@BlockIndex int rightPosition)
{
return isDistinctFrom(
valueDistinctFromFunction,
mapType,
(Block) mapType.getObject(left, leftPosition),
left.isNull(leftPosition),
(Block) mapType.getObject(right, rightPosition),
right.isNull(rightPosition));
}
}
30 changes: 25 additions & 5 deletions presto-main/src/main/java/com/facebook/presto/operator/scalar/RowDistinctFromOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.facebook.presto.metadata.FunctionRegistry;
import com.facebook.presto.metadata.Signature;
import com.facebook.presto.metadata.SqlOperator;
import com.facebook.presto.operator.scalar.ScalarFunctionImplementation.ScalarImplementationChoice;
import com.facebook.presto.spi.block.Block;
import com.facebook.presto.spi.function.InvocationConvention;
import com.facebook.presto.spi.type.StandardTypes;
Expand All @@ -31,6 +32,7 @@

import static com.facebook.presto.metadata.Signature.comparableWithVariadicBound;
import static com.facebook.presto.operator.scalar.ScalarFunctionImplementation.ArgumentProperty.valueTypeArgumentProperty;
import static com.facebook.presto.operator.scalar.ScalarFunctionImplementation.NullConvention.BLOCK_AND_POSITION;
import static com.facebook.presto.operator.scalar.ScalarFunctionImplementation.NullConvention.USE_NULL_FLAG;
import static com.facebook.presto.spi.function.InvocationConvention.InvocationArgumentConvention.NULL_FLAG;
import static com.facebook.presto.spi.function.OperatorType.IS_DISTINCT_FROM;
Expand All @@ -44,7 +46,8 @@ public class RowDistinctFromOperator
extends SqlOperator
{
public static final RowDistinctFromOperator ROW_DISTINCT_FROM = new RowDistinctFromOperator();
private static final MethodHandle METHOD_HANDLE = methodHandle(RowDistinctFromOperator.class, "isDistinctFrom", Type.class, List.class, Block.class, boolean.class, Block.class, boolean.class);
private static final MethodHandle METHOD_HANDLE_NULL_FLAG = methodHandle(RowDistinctFromOperator.class, "isDistinctFrom", Type.class, List.class, Block.class, boolean.class, Block.class, boolean.class);
private static final MethodHandle METHOD_HANDLE_BLOCK_POSITION = methodHandle(RowDistinctFromOperator.class, "isDistinctFrom", Type.class, List.class, Block.class, int.class, Block.class, int.class);

private RowDistinctFromOperator()
{
Expand All @@ -71,11 +74,17 @@ public ScalarFunctionImplementation specialize(BoundVariables boundVariables, in
argumentMethods.add(functionInvoker.methodHandle());
}
return new ScalarFunctionImplementation(
false,
ImmutableList.of(
valueTypeArgumentProperty(USE_NULL_FLAG),
valueTypeArgumentProperty(USE_NULL_FLAG)),
METHOD_HANDLE.bindTo(type).bindTo(argumentMethods.build()),
new ScalarImplementationChoice(
false,
ImmutableList.of(valueTypeArgumentProperty(USE_NULL_FLAG), valueTypeArgumentProperty(USE_NULL_FLAG)),
METHOD_HANDLE_NULL_FLAG.bindTo(type).bindTo(argumentMethods.build()),
Optional.empty()),
new ScalarImplementationChoice(
false,
ImmutableList.of(valueTypeArgumentProperty(BLOCK_AND_POSITION), valueTypeArgumentProperty(BLOCK_AND_POSITION)),
METHOD_HANDLE_BLOCK_POSITION.bindTo(type).bindTo(argumentMethods.build()),
Optional.empty())),
isDeterministic());
}

Expand Down Expand Up @@ -115,4 +124,15 @@ public static boolean isDistinctFrom(Type rowType, List<MethodHandle> argumentMe
}
return false;
}

public static boolean isDistinctFrom(Type rowType, List<MethodHandle> argumentMethods, Block leftRow, int leftPosition, Block rightRow, int rightPosition)
{
return isDistinctFrom(
rowType,
argumentMethods,
(Block) rowType.getObject(leftRow, leftPosition),
leftRow.isNull(leftPosition),
(Block) rowType.getObject(rightRow, rightPosition),
rightRow.isNull(rightPosition));
}
}
Loading