-
Notifications
You must be signed in to change notification settings - Fork 215
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
Added 'long' as a target type for 'convert_entry_type' processor #4359
Changes from 5 commits
0c4dde4
b11b660
7ad925b
d0aafe7
c3672e9
85d3b7d
4cbe747
bd81e92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.typeconverter; | ||
|
||
public class LongConverter implements TypeConverter<Long> { | ||
public Long convert(Object source) throws IllegalArgumentException { | ||
if (source instanceof String) { | ||
return Long.parseLong((String)source); | ||
} | ||
if (source instanceof Float) { | ||
return (long)(float)((Float)source); | ||
} | ||
if (source instanceof Double) { | ||
return (long)(double)((Double)source); | ||
} | ||
if (source instanceof Boolean) { | ||
return ((Boolean)source) ? 1L : 0L; | ||
} | ||
if (source instanceof Integer) { | ||
return ((Integer)source).longValue(); | ||
} | ||
if (source instanceof Long) { | ||
return (Long)source; | ||
} | ||
throw new IllegalArgumentException("Unsupported type conversion. Source class: " + source.getClass()); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,55 @@ | ||||
/* | ||||
* Copyright OpenSearch Contributors | ||||
* SPDX-License-Identifier: Apache-2.0 | ||||
*/ | ||||
|
||||
package org.opensearch.dataprepper.typeconverter; | ||||
|
||||
import org.junit.jupiter.api.Test; | ||||
|
||||
import static org.hamcrest.CoreMatchers.equalTo; | ||||
import static org.hamcrest.MatcherAssert.assertThat; | ||||
import static org.junit.jupiter.api.Assertions.assertThrows; | ||||
|
||||
public class LongConverterTests { | ||||
@Test | ||||
void testStringToLongConversion() { | ||||
LongConverter converter = new LongConverter(); | ||||
final String stringConstant = "1234"; | ||||
assertThat(converter.convert(stringConstant), equalTo(Long.parseLong(stringConstant))); | ||||
} | ||||
@Test | ||||
void testfloatToLongConversion() { | ||||
LongConverter converter = new LongConverter(); | ||||
final Float floatConstant = (float)1234.56789; | ||||
assertThat(converter.convert(floatConstant), equalTo((long)(float)floatConstant)); | ||||
} | ||||
@Test | ||||
void testDoubleToLongConversion() { | ||||
LongConverter converter = new LongConverter(); | ||||
final Double doubleConstant = (double)12345678.12345678; | ||||
assertThat(converter.convert(doubleConstant), equalTo((long)(double)doubleConstant)); | ||||
} | ||||
@Test | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These test cases look good. One thing you could do to make it easier to add more tests would be to use @ParameterizedTest with an Arguments source. An example of that can be found here ( Line 113 in 8cb5a84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the |
||||
void testBooleanToLongConversion() { | ||||
LongConverter converter = new LongConverter(); | ||||
final Boolean boolFalseConstant = false; | ||||
assertThat(converter.convert(boolFalseConstant), equalTo(0L)); | ||||
final Boolean boolTrueConstant = true; | ||||
assertThat(converter.convert(boolTrueConstant), equalTo(1L)); | ||||
} | ||||
@Test | ||||
void testLongToLongConversion() { | ||||
LongConverter converter = new LongConverter(); | ||||
final Long longConstant = (long)1234; | ||||
assertThat(converter.convert(longConstant), equalTo(longConstant)); | ||||
} | ||||
@Test | ||||
void testInvalidStringConversion() { | ||||
IntegerConverter converter = new IntegerConverter(); | ||||
assertThrows(IllegalArgumentException.class, () -> converter.convert(new Object())); | ||||
IntegerConverter converter = new IntegerConverter(); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm seeing the following failure:
I think you can remove lines 51 and 52 and they repeat the same tests from 49-50. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was a mistake on my part of using |
||||
assertThrows(IllegalArgumentException.class, () -> converter.convert(new Object())); | ||||
} | ||||
} | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be good to have a test that converts a value which is larger than the maximum integer in addition to this one. You could make a large double constant like
2.0 * Integer.MAX_INT
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test case
testDoubleToLongConversionWithBigValue
for it.