Skip to content

Bucketing fixes #1336

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
findepi merged 3 commits into from
Aug 21, 2019
Merged

Bucketing fixes #1336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd82206
Reuse constants
findepi Aug 20, 2019
df4bcae
Normalize float for Hive bucket hash
findepi Aug 20, 2019
d28898f
Hide $bucket column for bucketing v2
findepi Aug 20, 2019
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
8 changes: 6 additions & 2 deletions presto-hive/src/main/java/io/prestosql/plugin/hive/HiveBucketing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@
import static io.prestosql.plugin.hive.HiveErrorCode.HIVE_INVALID_METADATA;
import static io.prestosql.plugin.hive.HiveUtil.getRegularColumnHandles;
import static java.lang.Double.doubleToLongBits;
import static java.lang.Float.floatToIntBits;
import static java.lang.Float.intBitsToFloat;
import static java.lang.Math.toIntExact;
import static java.lang.String.format;
import static java.util.Map.Entry;
Expand Down Expand Up @@ -141,7 +143,8 @@ private static int hash(TypeInfo type, Block block, int position)
long bigintValue = prestoType.getLong(block, position);
return (int) ((bigintValue >>> 32) ^ bigintValue);
case FLOAT:
return (int) prestoType.getLong(block, position);
// convert to canonical NaN if necessary
Copy link
Copy Markdown
Member

@kokosing kokosing Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

can you add a test that for two different ints that represents NaN in float they are producing same hash? That would be much better than a comment in the code.

Copy link
Copy Markdown
Member Author

@findepi findepi Aug 20, 2019

Choose a reason for hiding this comment

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

I'd need a new connector where I can force a particular bits representation of a NaN value...

Copy link
Copy Markdown
Member Author

@findepi findepi Aug 20, 2019

Choose a reason for hiding this comment

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

Thanks for catching me. I added a unit test and it told me there's another method to update.

return floatToIntBits(intBitsToFloat(toIntExact(prestoType.getLong(block, position))));
case DOUBLE:
long doubleValue = doubleToLongBits(prestoType.getDouble(block, position));
return (int) ((doubleValue >>> 32) ^ doubleValue);
Expand Down Expand Up @@ -197,7 +200,8 @@ private static int hash(TypeInfo type, Object value)
long bigintValue = (long) value;
return (int) ((bigintValue >>> 32) ^ bigintValue);
case FLOAT:
return (int) (long) value;
// convert to canonical NaN if necessary
return floatToIntBits(intBitsToFloat(toIntExact((long) value)));
case DOUBLE:
long doubleValue = doubleToLongBits((double) value);
return (int) ((doubleValue >>> 32) ^ doubleValue);
Expand Down
3 changes: 2 additions & 1 deletion presto-hive/src/main/java/io/prestosql/plugin/hive/HiveUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
import static com.google.common.collect.Iterables.filter;
import static com.google.common.collect.Lists.newArrayList;
import static com.google.common.collect.Lists.transform;
import static io.prestosql.plugin.hive.HiveBucketing.isHiveBucketingV1;
import static io.prestosql.plugin.hive.HiveColumnHandle.ColumnType.PARTITION_KEY;
import static io.prestosql.plugin.hive.HiveColumnHandle.ColumnType.REGULAR;
import static io.prestosql.plugin.hive.HiveColumnHandle.bucketColumnHandle;
Expand Down Expand Up @@ -832,7 +833,7 @@ public static List<HiveColumnHandle> hiveColumnHandles(Table table)

// add hidden columns
columns.add(pathColumnHandle());
if (table.getStorage().getBucketProperty().isPresent()) {
if (table.getStorage().getBucketProperty().isPresent() && isHiveBucketingV1(table)) {
columns.add(bucketColumnHandle());
}

Expand Down
18 changes: 9 additions & 9 deletions presto-hive/src/main/java/io/prestosql/plugin/hive/s3/PrestoS3ClientFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,17 @@
import static com.amazonaws.regions.Regions.US_EAST_1;
import static com.google.common.base.Strings.isNullOrEmpty;
import static com.google.common.base.Verify.verify;
import static io.prestosql.plugin.hive.s3.PrestoS3FileSystem.S3_ACCESS_KEY;
import static io.prestosql.plugin.hive.s3.PrestoS3FileSystem.S3_CONNECT_TIMEOUT;
import static io.prestosql.plugin.hive.s3.PrestoS3FileSystem.S3_CREDENTIALS_PROVIDER;
import static io.prestosql.plugin.hive.s3.PrestoS3FileSystem.S3_ENDPOINT;
import static io.prestosql.plugin.hive.s3.PrestoS3FileSystem.S3_MAX_ERROR_RETRIES;
import static io.prestosql.plugin.hive.s3.PrestoS3FileSystem.S3_PIN_CLIENT_TO_CURRENT_REGION;
import static io.prestosql.plugin.hive.s3.PrestoS3FileSystem.S3_SECRET_KEY;
import static io.prestosql.plugin.hive.s3.PrestoS3FileSystem.S3_SOCKET_TIMEOUT;
import static io.prestosql.plugin.hive.s3.PrestoS3FileSystem.S3_SSL_ENABLED;
import static io.prestosql.plugin.hive.s3.PrestoS3FileSystem.S3_USER_AGENT_PREFIX;
import static io.prestosql.plugin.hive.s3.PrestoS3FileSystem.S3_USE_INSTANCE_CREDENTIALS;
import static java.lang.Math.toIntExact;
import static java.lang.String.format;

Expand All @@ -54,15 +63,6 @@
*/
public class PrestoS3ClientFactory
{
private static final String S3_ACCESS_KEY = "presto.s3.access-key";
private static final String S3_SECRET_KEY = "presto.s3.secret-key";
private static final String S3_CREDENTIALS_PROVIDER = "presto.s3.credentials-provider";
private static final String S3_USE_INSTANCE_CREDENTIALS = "presto.s3.use-instance-credentials";
private static final String S3_CONNECT_TIMEOUT = "presto.s3.connect-timeout";
private static final String S3_SOCKET_TIMEOUT = "presto.s3.socket-timeout";
private static final String S3_SSL_ENABLED = "presto.s3.ssl.enabled";
private static final String S3_MAX_ERROR_RETRIES = "presto.s3.max-error-retries";
private static final String S3_USER_AGENT_PREFIX = "presto.s3.user-agent-prefix";
private static final String S3_SELECT_PUSHDOWN_MAX_CONNECTIONS = "hive.s3select-pushdown.max-connections";
private static String s3UserAgentSuffix = "presto";

Expand Down
11 changes: 11 additions & 0 deletions presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveBucketing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static io.prestosql.plugin.hive.HiveTestUtils.TYPE_MANAGER;
import static io.prestosql.spi.type.TypeUtils.writeNativeValue;
import static java.lang.Double.longBitsToDouble;
import static java.lang.Float.intBitsToFloat;
import static java.util.Arrays.asList;
import static java.util.Map.Entry;
import static org.testng.Assert.assertEquals;
Expand Down Expand Up @@ -74,12 +76,21 @@ public void testHashingCompare()
assertBucketEquals("float", Float.MIN_VALUE, 1);
assertBucketEquals("float", Float.POSITIVE_INFINITY, 2139095040);
assertBucketEquals("float", Float.NEGATIVE_INFINITY, -8388608);
assertBucketEquals("float", Float.NaN, 2143289344);
assertBucketEquals("float", intBitsToFloat(0xffc00000), 2143289344); // also a NaN
assertBucketEquals("float", intBitsToFloat(0x7fc00000), 2143289344); // also a NaN
assertBucketEquals("float", intBitsToFloat(0x7fc01234), 2143289344); // also a NaN
assertBucketEquals("float", intBitsToFloat(0xffc01234), 2143289344); // also a NaN
assertBucketEquals("double", null, 0);
assertBucketEquals("double", 12.34, 986311098);
assertBucketEquals("double", -Double.MAX_VALUE, 1048576);
assertBucketEquals("double", Double.MIN_VALUE, 1);
assertBucketEquals("double", Double.POSITIVE_INFINITY, 2146435072);
assertBucketEquals("double", Double.NEGATIVE_INFINITY, -1048576);
assertBucketEquals("double", Double.NaN, 2146959360);
assertBucketEquals("double", longBitsToDouble(0xfff8000000000000L), 2146959360);
assertBucketEquals("double", longBitsToDouble(0x7ff8123412341234L), 2146959360);
assertBucketEquals("double", longBitsToDouble(0xfff8123412341234L), 2146959360);
assertBucketEquals("varchar(15)", null, 0);
assertBucketEquals("varchar(15)", "", 1);
assertBucketEquals("varchar(15)", "test string", -189841218);
Expand Down