Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
72 changes: 72 additions & 0 deletions core/src/main/java/org/apache/iceberg/StaticTableOperations.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*
*/

package org.apache.iceberg;

import java.nio.file.Paths;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.io.LocationProvider;

/**
* Representation of an immutable snapshot of Table State that can be used in
* other Iceberg Functions.
*/
public class StaticTableOperations implements TableOperations {
private final TableMetadata staticMetadata;
private final FileIO io;

/**
* Creates a StaticTableOperations tied to a specific static version of the TableMetadata
*/
public StaticTableOperations(String location, FileIO io) {
this.io = io;
this.staticMetadata = TableMetadataParser.read(io, location);
}

@Override
public TableMetadata current() {
return staticMetadata;
}

@Override
public TableMetadata refresh() {
return staticMetadata;
}

@Override
public void commit(TableMetadata base, TableMetadata metadata) {
throw new UnsupportedOperationException("This TableOperations is static, it cannot be modified");
}

@Override
public FileIO io() {
return this.io;
}

@Override
public String metadataFileLocation(String fileName) {
throw new UnsupportedOperationException("New files cannot be created in a Static Table Operations");
}

@Override
public LocationProvider locationProvider() {
throw new UnsupportedOperationException("New files cannot be created in a Static Table Operations");
}
}
38 changes: 22 additions & 16 deletions core/src/main/java/org/apache/iceberg/hadoop/HadoopTables.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.Map;
import org.apache.hadoop.conf.Configurable;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
import org.apache.iceberg.AllDataFilesTable;
import org.apache.iceberg.AllEntriesTable;
Expand All @@ -36,6 +37,7 @@
import org.apache.iceberg.PartitionsTable;
import org.apache.iceberg.Schema;
import org.apache.iceberg.SnapshotsTable;
import org.apache.iceberg.StaticTableOperations;
import org.apache.iceberg.Table;
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableOperations;
Expand All @@ -50,6 +52,7 @@
* to store metadata and manifests.
*/
public class HadoopTables implements Tables, Configurable {
private static final String METADATA_JSON = "metadata.json";
private Configuration conf;

public HadoopTables() {
Expand All @@ -68,27 +71,26 @@ public HadoopTables(Configuration conf) {
*/
@Override
public Table load(String location) {
TableOperations ops = newTableOps(location);
Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative to this implementation that I considered was modifying findTable itself. The problem with this is that to do that implementation we need to extract the loadMetadataTable from below. I think this is a cleaner solution but we could do something where we make a StaticTables.java which findTables uses to either return a basetable or metadataTable

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this. It is more straightforward than the previous implementation.

if (ops.current() == null) {
//Possibly Load a Metadata Table
Copy link
Contributor

Choose a reason for hiding this comment

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

How about parse out the metadata table name, if present?

Copy link
Member Author

Choose a reason for hiding this comment

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

I made a different implementation here where we will instead do

Name, Type = tryParseMetadataLocation

if (name, type).{
  loadMetadata
} else {
  loadBasic
}

if (location.contains("#") && !location.endsWith("#")) {
// try to resolve a metadata table, which we encode as URI fragments
// e.g. hdfs:///warehouse/my_table#snapshots
int hashIndex = location.lastIndexOf('#');
if (hashIndex != -1 && location.length() - 1 != hashIndex) {
// we found char '#', and it is not the last char of location
String baseTable = location.substring(0, hashIndex);
String metaTable = location.substring(hashIndex + 1);
MetadataTableType type = MetadataTableType.from(metaTable);
if (type != null) {
return loadMetadataTable(baseTable, type);
} else {
throw new NoSuchTableException("Table does not exist at location: " + location);
}
} else {
throw new NoSuchTableException("Table does not exist at location: " + location);
String baseTable = location.substring(0, hashIndex);
String metaTable = location.substring(hashIndex + 1);
MetadataTableType type = MetadataTableType.from(metaTable);
if (type != null) {
return loadMetadataTable(baseTable, type);
}
}

return new BaseTable(ops, location);
//Normal Table Load if we haven't loaded a MetadataTable
TableOperations ops = newTableOps(location);
if (ops.current() != null) {
return new BaseTable(ops, location);
} else {
throw new NoSuchTableException("Table does not exist at location: " + location);
}
}

private Table loadMetadataTable(String location, MetadataTableType type) {
Expand Down Expand Up @@ -152,7 +154,11 @@ public Table create(Schema schema, PartitionSpec spec, Map<String, String> prope
}

private TableOperations newTableOps(String location) {
return new HadoopTableOperations(new Path(location), conf);
if (location.contains(METADATA_JSON)) {
return new StaticTableOperations(location, new HadoopFileIO(conf));
} else {
return new HadoopTableOperations(new Path(location), conf);
}
}

@Override
Expand Down
79 changes: 79 additions & 0 deletions core/src/test/java/org/apache/iceberg/hadoop/TestStaticTable.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package org.apache.iceberg.hadoop;

import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import org.apache.iceberg.HasTableOperations;
import org.apache.iceberg.MetadataTableType;
import org.apache.iceberg.StaticTableOperations;
import org.apache.iceberg.Table;
import org.junit.Assert;
import org.junit.Test;

public class TestStaticTable extends HadoopTableTestBase {

private Table getStaticTable() {
return TABLES.load(((HasTableOperations) table).operations().current().metadataFileLocation());
}

private Table getStaticTable(MetadataTableType type) {
return TABLES.load(((HasTableOperations) table).operations().current().metadataFileLocation() + "#" + type);
}

@Test
public void testLoadFromMetadata() {
Table staticTable = getStaticTable();
Assert.assertTrue("Loading a metadata file based table should return StaticTableOperations",
((HasTableOperations) staticTable).operations() instanceof StaticTableOperations);
}

@Test(expected = UnsupportedOperationException.class)
public void testCannotBeAddedTo(){
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need both this and the next test, but it's okay to have them.

Table staticTable = getStaticTable();
staticTable.newOverwrite().addFile(FILE_A).commit();
}

@Test(expected = UnsupportedOperationException.class)
public void testCannotBeDeletedFrom(){
table.newAppend().appendFile(FILE_A).commit();
Table staticTable = getStaticTable();
staticTable.newDelete().deleteFile(FILE_A).commit();
}

@Test
public void testHasSameProperties(){
table.newAppend().appendFile(FILE_A).commit();
table.newAppend().appendFile(FILE_B).commit();
table.newOverwrite().deleteFile(FILE_B).addFile(FILE_C).commit();
Table staticTable = getStaticTable();
Assert.assertTrue("Same history?",
table.history().containsAll(staticTable.history()));
Assert.assertTrue("Same snapshot?",
table.currentSnapshot().snapshotId() == staticTable.currentSnapshot().snapshotId());
Assert.assertTrue("Same properties?",
Maps.difference(table.properties(), staticTable.properties()).areEqual());
}

@Test
public void testImmutable() {
table.newAppend().appendFile(FILE_A).commit();
Table staticTable = getStaticTable();
long originalSnapshot = table.currentSnapshot().snapshotId();

table.newAppend().appendFile(FILE_B).commit();
table.newOverwrite().deleteFile(FILE_B).addFile(FILE_C).commit();

Assert.assertEquals("Snapshot unchanged after table modified",
staticTable.currentSnapshot().snapshotId(), originalSnapshot);
}

@Test
public void testMetadataTables() {
for (MetadataTableType type: MetadataTableType.values()) {
String enumName = type.name().replace("_","").toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

When is this needed?

Also, style nit: no space between arguments to replace.

Copy link
Member Author

@RussellSpitzer RussellSpitzer Aug 14, 2020

Choose a reason for hiding this comment

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

ALL_DATA_FILES, ALL_MANIFESTS, ALL_ENTRIES

All the style things just make me want to upgrade the Checkstyle version even more :) I forgot to run in Intellij where I can force the newer version

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought those were loaded using all_data_files, all_manifests, etc. and not alldatafiles.

Copy link
Member Author

Choose a reason for hiding this comment

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

we are just comparing the ClassName here, so we are comparing AllDataFilesTable.class with ALL_DATA_FILES

I may be misunderstanding your comment here, I'm just making sure that we get back the Class with the right name based on the MetadataType we request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I see. Nevermind, then.

Assert.assertTrue("Should be able to get MetadataTable of type : " + type,
getStaticTable(type).getClass().getName().toLowerCase().contains(enumName));
}
}


}