Skip to content

HADOOP-18029: Update CompressionCodecFactory to handle uppercase file extensions #3739

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
giovannifumarola merged 5 commits into from
Dec 1, 2021
Merged

HADOOP-18029: Update CompressionCodecFactory to handle uppercase file extensions #3739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e529026
Lowercase the filename comparison in getCodec()
Desmeister Nov 30, 2021
e3e4967
New tests against capitalized filenames, added rich error messages ov…
Desmeister Nov 30, 2021
7e2c848
Fix error message for expected null
Desmeister Dec 1, 2021
917f0b3
Remove StringUtils import and separate lowercase filename
Desmeister Dec 1, 2021
225c7df
Fix all checkstyle issues in changed files.
Desmeister Dec 1, 2021
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
140 changes: 74 additions & 66 deletions ...ct/hadoop-common/src/main/java/org/apache/hadoop/io/compress/CompressionCodecFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public class CompressionCodecFactory {
LoggerFactory.getLogger(CompressionCodecFactory.class.getName());

private static final ServiceLoader<CompressionCodec> CODEC_PROVIDERS =
ServiceLoader.load(CompressionCodec.class);
ServiceLoader.load(CompressionCodec.class);

/**
* A map from the reversed filename suffixes to the codecs.
Expand All @@ -49,15 +49,15 @@ public class CompressionCodecFactory {
*/
private SortedMap<String, CompressionCodec> codecs = null;

/**
* A map from the reversed filename suffixes to the codecs.
* This is probably overkill, because the maps should be small, but it
* automatically supports finding the longest matching suffix.
*/
private Map<String, CompressionCodec> codecsByName = null;
/**
* A map from the reversed filename suffixes to the codecs.
* This is probably overkill, because the maps should be small, but it
* automatically supports finding the longest matching suffix.
*/
private Map<String, CompressionCodec> codecsByName = null;

/**
* A map from class names to the codecs
* A map from class names to the codecs.
*/
private HashMap<String, CompressionCodec> codecsByClassName = null;

Expand All @@ -80,8 +80,8 @@ private void addCodec(CompressionCodec codec) {
@Override
public String toString() {
StringBuilder buf = new StringBuilder();
Iterator<Map.Entry<String, CompressionCodec>> itr =
codecs.entrySet().iterator();
Iterator<Map.Entry<String, CompressionCodec>> itr =
codecs.entrySet().iterator();
buf.append("{ ");
if (itr.hasNext()) {
Map.Entry<String, CompressionCodec> entry = itr.next();
Expand Down Expand Up @@ -110,8 +110,8 @@ public String toString() {
*/
public static List<Class<? extends CompressionCodec>> getCodecClasses(
Configuration conf) {
List<Class<? extends CompressionCodec>> result
= new ArrayList<Class<? extends CompressionCodec>>();
List<Class<? extends CompressionCodec>> result =
new ArrayList<Class<? extends CompressionCodec>>();
// Add codec classes discovered via service loading
synchronized (CODEC_PROVIDERS) {
// CODEC_PROVIDERS is a lazy collection. Synchronize so it is
Expand Down Expand Up @@ -200,11 +200,13 @@ public CompressionCodec getCodec(Path file) {
String filename = file.getName();
String reversedFilename =
new StringBuilder(filename).reverse().toString();
SortedMap<String, CompressionCodec> subMap =
codecs.headMap(reversedFilename);
String lowerReversedFilename =
StringUtils.toLowerCase(reversedFilename);
SortedMap<String, CompressionCodec> subMap =
codecs.headMap(lowerReversedFilename);
if (!subMap.isEmpty()) {
String potentialSuffix = subMap.lastKey();
if (reversedFilename.startsWith(potentialSuffix)) {
if (lowerReversedFilename.startsWith(potentialSuffix)) {
result = codecs.get(potentialSuffix);
}
}
Expand All @@ -224,57 +226,57 @@ public CompressionCodec getCodecByClassName(String classname) {
return codecsByClassName.get(classname);
}

/**
* Find the relevant compression codec for the codec's canonical class name
* or by codec alias.
* <p>
* Codec aliases are case insensitive.
* <p>
* The code alias is the short class name (without the package name).
* If the short class name ends with 'Codec', then there are two aliases for
* the codec, the complete short class name and the short class name without
* the 'Codec' ending. For example for the 'GzipCodec' codec class name the
* alias are 'gzip' and 'gzipcodec'.
*
* @param codecName the canonical class name of the codec
* @return the codec object
*/
public CompressionCodec getCodecByName(String codecName) {
if (codecsByClassName == null) {
return null;
}
CompressionCodec codec = getCodecByClassName(codecName);
if (codec == null) {
// trying to get the codec by name in case the name was specified
// instead a class
codec = codecsByName.get(StringUtils.toLowerCase(codecName));
}
return codec;
/**
* Find the relevant compression codec for the codec's canonical class name
* or by codec alias.
* <p>
* Codec aliases are case insensitive.
* <p>
* The code alias is the short class name (without the package name).
* If the short class name ends with 'Codec', then there are two aliases for
* the codec, the complete short class name and the short class name without
* the 'Codec' ending. For example for the 'GzipCodec' codec class name the
* alias are 'gzip' and 'gzipcodec'.
*
* @param codecName the canonical class name of the codec
* @return the codec object
*/
public CompressionCodec getCodecByName(String codecName) {
if (codecsByClassName == null) {
return null;
}
CompressionCodec codec = getCodecByClassName(codecName);
if (codec == null) {
// trying to get the codec by name in case the name was specified
// instead a class
codec = codecsByName.get(StringUtils.toLowerCase(codecName));
}
return codec;
}

/**
* Find the relevant compression codec for the codec's canonical class name
* or by codec alias and returns its implemetation class.
* <p>
* Codec aliases are case insensitive.
* <p>
* The code alias is the short class name (without the package name).
* If the short class name ends with 'Codec', then there are two aliases for
* the codec, the complete short class name and the short class name without
* the 'Codec' ending. For example for the 'GzipCodec' codec class name the
* alias are 'gzip' and 'gzipcodec'.
*
* @param codecName the canonical class name of the codec
* @return the codec class
*/
public Class<? extends CompressionCodec> getCodecClassByName(
String codecName) {
CompressionCodec codec = getCodecByName(codecName);
if (codec == null) {
return null;
}
return codec.getClass();
/**
* Find the relevant compression codec for the codec's canonical class name
* or by codec alias and returns its implemetation class.
* <p>
* Codec aliases are case insensitive.
* <p>
* The code alias is the short class name (without the package name).
* If the short class name ends with 'Codec', then there are two aliases for
* the codec, the complete short class name and the short class name without
* the 'Codec' ending. For example for the 'GzipCodec' codec class name the
* alias are 'gzip' and 'gzipcodec'.
*
* @param codecName the canonical class name of the codec
* @return the codec class
*/
public Class<? extends CompressionCodec> getCodecClassByName(
String codecName) {
CompressionCodec codec = getCodecByName(codecName);
if (codec == null) {
return null;
}
return codec.getClass();
}

/**
* Removes a suffix from a filename, if it has it.
Expand Down Expand Up @@ -323,8 +325,12 @@ public static void main(String[] args) throws Exception {
len = in.read(buffer);
}
} finally {
if(out != null) { out.close(); }
if(in != null) { in.close(); }
if(out != null) {
out.close();
}
if(in != null) {
in.close();
}
}
} else {
CompressionInputStream in = null;
Expand All @@ -338,7 +344,9 @@ public static void main(String[] args) throws Exception {
len = in.read(buffer);
}
} finally {
if(in != null) { in.close(); }
if(in != null) {
in.close();
}
}
}
}
Expand Down
49 changes: 36 additions & 13 deletions ...n-project/hadoop-common/src/test/java/org/apache/hadoop/io/compress/TestCodecFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import org.junit.Test;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.fail;

public class TestCodecFactory {
Expand All @@ -45,8 +46,8 @@ public Configuration getConf() {
}

@Override
public CompressionOutputStream createOutputStream(OutputStream out)
throws IOException {
public CompressionOutputStream createOutputStream(OutputStream out)
throws IOException {
return null;
}

Expand All @@ -62,21 +63,21 @@ public Compressor createCompressor() {

@Override
public CompressionInputStream createInputStream(InputStream in,
Decompressor decompressor)
throws IOException {
Decompressor decompressor)
throws IOException {
return null;
}

@Override
public CompressionInputStream createInputStream(InputStream in)
throws IOException {
public CompressionInputStream createInputStream(InputStream in)
throws IOException {
return null;
}

@Override
public CompressionOutputStream createOutputStream(OutputStream out,
Compressor compressor)
throws IOException {
Compressor compressor)
throws IOException {
return null;
}

Expand Down Expand Up @@ -125,7 +126,7 @@ public String getDefaultExtension() {
}

/**
* Returns a factory for a given set of codecs
* Returns a factory for a given set of codecs.
* @param classes the codec classes to include
* @return a new factory
*/
Expand All @@ -137,22 +138,30 @@ private static CompressionCodecFactory setClasses(Class[] classes) {

private static void checkCodec(String msg,
Class expected, CompressionCodec actual) {
assertEquals(msg + " unexpected codec found",
expected.getName(),
actual.getClass().getName());
if (expected == null) {
assertNull(msg, actual);
} else if (actual == null) {
fail(msg + " result was null");
} else {
assertEquals(msg + " unexpected codec found",
expected.getName(),
actual.getClass().getName());
}
}

@Test
public void testFinding() {
CompressionCodecFactory factory =
new CompressionCodecFactory(new Configuration());
new CompressionCodecFactory(new Configuration());
CompressionCodec codec = factory.getCodec(new Path("/tmp/foo.bar"));
assertEquals("default factory foo codec", null, codec);
codec = factory.getCodecByClassName(BarCodec.class.getCanonicalName());
assertEquals("default factory foo codec", null, codec);

codec = factory.getCodec(new Path("/tmp/foo.gz"));
checkCodec("default factory for .gz", GzipCodec.class, codec);
codec = factory.getCodec(new Path("/tmp/foo.GZ"));
checkCodec("default factory for .GZ", GzipCodec.class, codec);
codec = factory.getCodecByClassName(GzipCodec.class.getCanonicalName());
checkCodec("default factory for gzip codec", GzipCodec.class, codec);
codec = factory.getCodecByName("gzip");
Expand All @@ -168,6 +177,8 @@ public void testFinding() {

codec = factory.getCodec(new Path("/tmp/foo.bz2"));
checkCodec("default factory for .bz2", BZip2Codec.class, codec);
codec = factory.getCodec(new Path("/tmp/foo.BZ2"));
checkCodec("default factory for .BZ2", BZip2Codec.class, codec);
codec = factory.getCodecByClassName(BZip2Codec.class.getCanonicalName());
checkCodec("default factory for bzip2 codec", BZip2Codec.class, codec);
codec = factory.getCodecByName("bzip2");
Expand Down Expand Up @@ -221,16 +232,22 @@ public void testFinding() {
FooBarCodec.class});
codec = factory.getCodec(new Path("/tmp/.foo.bar.gz"));
checkCodec("full factory gz codec", GzipCodec.class, codec);
codec = factory.getCodec(new Path("/tmp/.foo.bar.GZ"));
checkCodec("full factory GZ codec", GzipCodec.class, codec);
codec = factory.getCodecByClassName(GzipCodec.class.getCanonicalName());
checkCodec("full codec gz codec", GzipCodec.class, codec);

codec = factory.getCodec(new Path("/tmp/foo.bz2"));
checkCodec("full factory for .bz2", BZip2Codec.class, codec);
codec = factory.getCodec(new Path("/tmp/foo.BZ2"));
checkCodec("full factory for .BZ2", BZip2Codec.class, codec);
codec = factory.getCodecByClassName(BZip2Codec.class.getCanonicalName());
checkCodec("full codec bzip2 codec", BZip2Codec.class, codec);

codec = factory.getCodec(new Path("/tmp/foo.bar"));
checkCodec("full factory bar codec", BarCodec.class, codec);
codec = factory.getCodec(new Path("/tmp/foo.BAR"));
checkCodec("full factory BAR codec", BarCodec.class, codec);
codec = factory.getCodecByClassName(BarCodec.class.getCanonicalName());
checkCodec("full factory bar codec", BarCodec.class, codec);
codec = factory.getCodecByName("bar");
Expand All @@ -240,6 +257,8 @@ public void testFinding() {

codec = factory.getCodec(new Path("/tmp/foo/baz.foo.bar"));
checkCodec("full factory foo bar codec", FooBarCodec.class, codec);
codec = factory.getCodec(new Path("/tmp/foo/baz.FOO.bar"));
checkCodec("full factory FOO bar codec", FooBarCodec.class, codec);
codec = factory.getCodecByClassName(FooBarCodec.class.getCanonicalName());
checkCodec("full factory foo bar codec", FooBarCodec.class, codec);
codec = factory.getCodecByName("foobar");
Expand All @@ -249,6 +268,8 @@ public void testFinding() {

codec = factory.getCodec(new Path("/tmp/foo.foo"));
checkCodec("full factory foo codec", FooCodec.class, codec);
codec = factory.getCodec(new Path("/tmp/FOO.FOO"));
checkCodec("full factory FOO codec", FooCodec.class, codec);
codec = factory.getCodecByClassName(FooCodec.class.getCanonicalName());
checkCodec("full factory foo codec", FooCodec.class, codec);
codec = factory.getCodecByName("foo");
Expand All @@ -259,6 +280,8 @@ public void testFinding() {
factory = setClasses(new Class[]{NewGzipCodec.class});
codec = factory.getCodec(new Path("/tmp/foo.gz"));
checkCodec("overridden factory for .gz", NewGzipCodec.class, codec);
codec = factory.getCodec(new Path("/tmp/foo.GZ"));
checkCodec("overridden factory for .GZ", NewGzipCodec.class, codec);
codec = factory.getCodecByClassName(NewGzipCodec.class.getCanonicalName());
checkCodec("overridden factory for gzip codec", NewGzipCodec.class, codec);

Expand Down