Skip to content

Commit 0eaff64

Browse files
shrinidhijoshiPresto CUDF CI
authored andcommitted
[pos][native] Fix unit testing setup for pos native tests
Makes below changes to ensure pos native tests run correctly - Configure the test runner - PrestoSparkRunner - to use hive-admin identity so that we are compliant with sql-standard access control. Without this, the query result assertions will fail with ACL errors - Temporarily duplicated utility function 0 HiveTestUtils.getProperty to circumvent instantiation of HiveTestUtils class instantiation issues. To be fixed in a separate PR
1 parent 670094f commit 0eaff64

File tree

3 files changed

+47
-4
lines changed

3 files changed

+47
-4
lines changed

presto-native-execution/src/test/java/com/facebook/presto/spark/PrestoSparkNativeQueryRunnerUtils.java

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,11 @@
3434
import java.util.Optional;
3535

3636
import static com.facebook.airlift.log.Level.WARN;
37-
import static com.facebook.presto.hive.HiveTestUtils.getProperty;
3837
import static com.facebook.presto.nativeworker.NativeQueryRunnerUtils.getNativeWorkerHiveProperties;
3938
import static com.facebook.presto.nativeworker.NativeQueryRunnerUtils.getNativeWorkerSystemProperties;
4039
import static com.facebook.presto.nativeworker.PrestoNativeQueryRunnerUtils.getNativeQueryRunnerParameters;
4140
import static com.facebook.presto.spark.PrestoSparkQueryRunner.METASTORE_CONTEXT;
41+
import static java.lang.String.format;
4242
import static java.nio.file.Files.createTempDirectory;
4343

4444
/**
@@ -138,7 +138,7 @@ public static PrestoSparkQueryRunner createRunner(String defaultCatalog, Optiona
138138
PrestoSparkQueryRunner queryRunner = new PrestoSparkQueryRunner(
139139
defaultCatalog,
140140
configBuilder.build(),
141-
getNativeWorkerHiveProperties(DEFAULT_STORAGE_FORMAT),
141+
getNativeWorkerHiveProperties(),
142142
additionalSparkProperties,
143143
dataDir,
144144
nativeModules,
@@ -154,7 +154,10 @@ public static PrestoSparkQueryRunner createRunner(String defaultCatalog, Optiona
154154
public static QueryRunner createJavaQueryRunner()
155155
throws Exception
156156
{
157-
return PrestoNativeQueryRunnerUtils.createJavaQueryRunner(Optional.of(getBaseDataPath()), "legacy", DEFAULT_STORAGE_FORMAT, true);
157+
return PrestoNativeQueryRunnerUtils.javaHiveQueryRunnerBuilder()
158+
.setAddStorageFormatToPath(true)
159+
.setStorageFormat(DEFAULT_STORAGE_FORMAT)
160+
.build();
158161
}
159162

160163
public static void customizeLogging()
@@ -201,4 +204,34 @@ public static synchronized Path getBaseDataPath()
201204
}
202205
return dataDirectory.get();
203206
}
207+
208+
// This is a temporary replacement for the function from
209+
// HiveTestUtils.getProperty. But HiveTestUtils instantiation seems to be
210+
// failing due to missing info in Hdfs setup. Until we fix that, this is a
211+
// copy of that function. As its a simple utility function, we are ok to punt
212+
// fixing this
213+
// TODO: Use HiveTestUtils.getProperty and delete this function
214+
public static Optional<String> getProperty(String name)
215+
{
216+
String systemPropertyValue = System.getProperty(name);
217+
String environmentVariableValue = System.getenv(name);
218+
if (systemPropertyValue == null) {
219+
if (environmentVariableValue == null) {
220+
return Optional.empty();
221+
}
222+
else {
223+
return Optional.of(environmentVariableValue);
224+
}
225+
}
226+
else {
227+
if (environmentVariableValue != null && !systemPropertyValue.equals(environmentVariableValue)) {
228+
throw new IllegalArgumentException(format("%s is set in both Java system property and environment variable, but their values are different. The Java system property value is %s, while the" +
229+
" environment variable value is %s. Please use only one value.",
230+
name,
231+
systemPropertyValue,
232+
environmentVariableValue));
233+
}
234+
return Optional.of(systemPropertyValue);
235+
}
236+
}
204237
}

presto-spark-base/pom.xml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,10 @@
6969
<groupId>com.facebook.presto</groupId>
7070
<artifactId>presto-main-base</artifactId>
7171
</dependency>
72-
72+
<dependency>
73+
<groupId>com.facebook.presto</groupId>
74+
<artifactId>presto-main</artifactId>
75+
</dependency>
7376
<dependency>
7477
<groupId>com.facebook.presto</groupId>
7578
<artifactId>presto-expressions</artifactId>

presto-spark-base/src/test/java/com/facebook/presto/spark/PrestoSparkQueryRunner.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@
6363
import com.facebook.presto.spi.WarningCollector;
6464
import com.facebook.presto.spi.eventlistener.EventListener;
6565
import com.facebook.presto.spi.function.FunctionImplementationType;
66+
import com.facebook.presto.spi.security.Identity;
6667
import com.facebook.presto.spi.security.PrincipalType;
6768
import com.facebook.presto.split.PageSourceManager;
6869
import com.facebook.presto.split.SplitManager;
@@ -330,6 +331,12 @@ public PrestoSparkQueryRunner(
330331
defaultSession = testSessionBuilder(injector.getInstance(SessionPropertyManager.class))
331332
.setCatalog(defaultCatalog)
332333
.setSchema("tpch")
334+
// Sql-Standard Access Control Checker
335+
// needs us to specify our role
336+
.setIdentity(new Identity(
337+
"hive",
338+
Optional.empty(),
339+
ImmutableMap.of(defaultCatalog, "admin")))
333340
.build();
334341

335342
transactionManager = injector.getInstance(TransactionManager.class);

0 commit comments

Comments
 (0)