Skip to content
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.

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

[Iceberg] Add Histogram Statistic Support #22365

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
204 changes: 201 additions & 3 deletions presto-common/src/main/java/com/facebook/presto/common/Utils.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,15 @@
import com.facebook.presto.common.predicate.Primitives;
import com.facebook.presto.common.type.Type;

import javax.annotation.Nullable;

import java.util.Arrays;
import java.util.function.Supplier;

import static com.facebook.presto.common.type.TypeUtils.readNativeValue;
import static com.facebook.presto.common.type.TypeUtils.writeNativeValue;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public final class Utils
{
Expand All @@ -30,7 +37,7 @@ private Utils()
public static Block nativeValueToBlock(Type type, Object object)
{
if (object != null && !Primitives.wrap(type.getJavaType()).isInstance(object)) {
throw new IllegalArgumentException(String.format("Object '%s' does not match type %s", object, type.getJavaType()));
throw new IllegalArgumentException(format("Object '%s' does not match type %s", object, type.getJavaType()));
}
BlockBuilder blockBuilder = type.createBlockBuilder(null, 1);
writeNativeValue(type, blockBuilder, object);
Expand All @@ -49,10 +56,201 @@ public static void checkArgument(boolean expression)
}
}

public static void checkArgument(boolean expression, String errorMessage)
public static void checkArgument(boolean expression, String message, Object... args)
ZacBlanco marked this conversation as resolved.
Show resolved Hide resolved
{
if (!expression) {
throw new IllegalArgumentException(errorMessage);
throw new IllegalArgumentException(format(message, args));
}
}

/**
* Returns a supplier which caches the instance retrieved during the first call to {@code get()}
* and returns that value on subsequent calls to {@code get()}.
*/
public static <T> Supplier<T> memoizedSupplier(Supplier<T> delegate)
ZacBlanco marked this conversation as resolved.
Show resolved Hide resolved
{
if (delegate instanceof MemoizingSupplier) {
return delegate;
}
return new MemoizingSupplier<>(delegate);
}

/**
* Vendored from Guava
*/
static class MemoizingSupplier<T>
implements Supplier<T>
{
volatile Supplier<T> delegate;
volatile boolean initialized;
// "value" does not need to be volatile; visibility piggy-backs
// on volatile read of "initialized".
@Nullable T value;

MemoizingSupplier(Supplier<T> delegate)
{
this.delegate = requireNonNull(delegate);
}

@Override
public T get()
{
// A 2-field variant of Double Checked Locking.
if (!initialized) {
synchronized (this) {
if (!initialized) {
T t = delegate.get();
value = t;
initialized = true;
// Release the delegate to GC.
delegate = null;
return t;
}
}
}
return value;
}

@Override
public String toString()
{
Supplier<T> delegate = this.delegate;
return "Suppliers.memoize("
+ (delegate == null ? "<supplier that returned " + value + ">" : delegate)
+ ")";
}
}

public static ToStringHelper toStringHelper(Object self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's attribute this to Guava ?

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we just use Guava for all of this? We already depend on it. If there is some reason we want to fork this code, then full tests are needed. But it should be much easier just to call Guava

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The guava dependency is not included in presto-common as the presto-common module is a dependency of the presto-spi module which we avoid putting additional dependencies so that when creating connectors or other plugins there it is less likely for downstream users of presto to encounter issues or conflicts

Copy link
Contributor

@elharo elharo Oct 5, 2024

Choose a reason for hiding this comment

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

OK, that's reasonable. However if we're forking this in, we also MUST include the tests as well, and be prepared to support this for the foreseeable future. I'm not sure it's worth the effort. I'd be inclined to just manually build the strings in the toString method like we've been doing since 1995. :-)

Choose a reason for hiding this comment

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

If we really need guava without disrupting the downstream users of the library - we could consider shading it (during the build process)? or even create a separate library e.g. presto-shaded-guava. There is an extra effort in testing and maintaining this code.

@ZacBlanco is this already considered?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, let's not do that. Shading is so painful. I think I'm coming down pretty firmly on the side of don't use ToStringHelper, don't use Guava here, and just concatenate the strings inside toString. It's not like that's actually hard. This approach is way too complicated for the problem it solves.

https://jlbp.dev/JLBP-18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ScrapCodes, as @elharo mentioned, shading is definitely a last resort and we should avoid it as much as possible. It's not worth doing. Guava is a huge codebase and would add a lot of bloat just for a few classes. I think vendoring these small utilities are useful though

{
return new ToStringHelper(self.getClass().getSimpleName());
}

public static ToStringHelper toStringHelper(Class<?> self)
{
return new ToStringHelper(self.getSimpleName());
}

public static ToStringHelper toStringHelper(String className)
{
return new ToStringHelper(className);
}

/**
* Vendored class from Guava.
*/
public static final class ToStringHelper
{
private final String className;
private final ValueHolder holderHead = new ValueHolder();
private ValueHolder holderTail = holderHead;
private boolean omitNullValues;

private ToStringHelper(String className)
{
this.className = requireNonNull(className);
}

public ToStringHelper omitNullValues()
{
omitNullValues = true;
return this;
}

public ToStringHelper add(String name, @Nullable Object value)
{
return addHolder(name, value);
}

public ToStringHelper add(String name, boolean value)
{
return addHolder(name, String.valueOf(value));
}

public ToStringHelper add(String name, char value)
{
return addHolder(name, String.valueOf(value));
}

public ToStringHelper add(String name, double value)
{
return addHolder(name, String.valueOf(value));
}

public ToStringHelper add(String name, float value)
{
return addHolder(name, String.valueOf(value));
}

public ToStringHelper add(String name, int value)
{
return addHolder(name, String.valueOf(value));
}

public ToStringHelper add(String name, long value)
{
return addHolder(name, String.valueOf(value));
}

@Override
public String toString()
{
// create a copy to keep it consistent in case value changes
boolean omitNullValuesSnapshot = omitNullValues;
String nextSeparator = "";
StringBuilder builder = new StringBuilder(32).append(className).append('{');
for (ValueHolder valueHolder = holderHead.next;
valueHolder != null;
valueHolder = valueHolder.next) {
Object value = valueHolder.value;
if (!omitNullValuesSnapshot || value != null) {
builder.append(nextSeparator);
nextSeparator = ", ";

if (valueHolder.name != null) {
builder.append(valueHolder.name).append('=');
}
if (value != null && value.getClass().isArray()) {
Object[] objectArray = {value};
String arrayString = Arrays.deepToString(objectArray);
builder.append(arrayString, 1, arrayString.length() - 1);
}
else {
builder.append(value);
}
}
}
return builder.append('}').toString();
}

private ValueHolder addHolder()
{
ValueHolder valueHolder = new ValueHolder();
holderTail.next = valueHolder;
holderTail = valueHolder;
return valueHolder;
}

private ToStringHelper addHolder(@Nullable Object value)
{
ValueHolder valueHolder = addHolder();
valueHolder.value = value;
return this;
}

private ToStringHelper addHolder(String name, @Nullable Object value)
{
ValueHolder valueHolder = addHolder();
valueHolder.value = value;
valueHolder.name = requireNonNull(name);
return this;
}

private static final class ValueHolder
{
@Nullable String name;
@Nullable Object value;
@Nullable ValueHolder next;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@ public Object getValue()
return Utils.blockToNativeValue(type, valueBlock.get());
}

public Optional<Object> getObjectValue()
ZacBlanco marked this conversation as resolved.
Show resolved Hide resolved
{
return valueBlock.map(block -> Utils.blockToNativeValue(type, block));
}

public Object getPrintableValue(SqlFunctionProperties properties)
{
if (!valueBlock.isPresent()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,16 @@ public boolean equals(Object obj)
Objects.equals(this.high, other.high);
}

@Override
public String toString()
{
return (low.getBound() == Marker.Bound.EXACTLY ? "[" : "(") +
low.getObjectValue().orElse(Double.NEGATIVE_INFINITY) +
".." +
high.getObjectValue().orElse(Double.POSITIVE_INFINITY) +
(high.getBound() == Marker.Bound.EXACTLY ? "]" : ")");
}

private void appendQuotedValue(StringBuilder buffer, Marker marker, SqlFunctionProperties properties)
{
buffer.append('"');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,28 @@ public Object getSingleValue()
return lowIndexedRanges.values().iterator().next().getSingleValue();
}

/**
* Build a new {@link SortedRangeSet} that contains ranges which lie within the argument range
*
* @param span the range which the new set should span
* @return a new range set
*/
public SortedRangeSet subRangeSet(Range span)
{
Builder builder = new Builder(type);

for (Range range : getOrderedRanges()) {
if (span.contains(range)) {
builder.add(range);
}
else if (span.overlaps(range)) {
builder.add(range.intersect(span));
}
}

return builder.build();
}

@Override
public boolean containsValue(Object value)
{
Expand Down
Loading
Loading