Xls addition #9

psainics · 2023-11-30T08:16:52Z

TODO: Add a description

vikasrathee-cs · 2023-12-11T04:22:26Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormat.java

+    public void initialize(InputSplit genericSplit, TaskAttemptContext context) throws IOException {
+
+      CombineFileSplit split = (CombineFileSplit) genericSplit;
+      Configuration job = context.getConfiguration();


change variable name to jobConf

vikasrathee-cs · 2023-12-11T04:35:20Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormatConfig.java

+   *
+   * @return A hashmap of column names and their manually set schemas.
+   */
+  public Map<String, Schema> getOverride() throws IllegalArgumentException {


Do we have this in all file plugins?

vikasrathee-cs · 2023-12-11T04:39:34Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormatDataFormatter.java

+      case ERROR:
+        return null;
+      default:
+        throw new IllegalStateException("Unexpected celltype (" + cellType + ")");


Use String.format

vikasrathee-cs · 2023-12-11T04:42:37Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormatProvider.java

+  }
+
+  @Override
+  protected void addFormatProperties(Map<String, String> properties) {


Add this property also to keep split as 1.
properties.put(FileInputFormat.SPLIT_MINSIZE, Long.toString(Long.MAX_VALUE));

vikasrathee-cs · 2023-12-11T04:44:29Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormatProvider.java

+      context.getFailureCollector().addFailure("Sheet number must be a number.", null)
+              .withConfigProperty(XlsInputFormatConfig.NAME_SHEET_VALUE);
+    }
+    FailureCollector collector = context.getFailureCollector();


If we are evaluating the schema from the file, when this will be triggered.

vikasrathee-cs · 2023-12-11T04:56:31Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormatUtils.java

+ * Utilities around XLS input format.
+ */
+public class XlsInputFormatUtils {
+  private static final Pattern NOT_VALID_PATTERN = Pattern.compile("[^A-Za-z0-9_]+");


Use the latest naming convention for BQ

This impl is taken from file plugin.
This is the invalid, column for CDAP.
We are not sure of the side effect this may cause, are you sure ?

anup-cloudsufi

Please add some unit tests before raising the external PR for review.

anup-cloudsufi · 2023-12-11T11:06:23Z

format-xls/pom.xml

+    <version>2.12.0-SNAPSHOT</version>
+  </parent>
+  <artifactId>format-xls</artifactId>
+  <name>XLSX format plugins</name>


Lets be consistent with format XLS.

Fixed !

XLS format plugins

anup-cloudsufi · 2023-12-11T11:07:12Z

format-xls/pom.xml

+    <dependency>
+      <groupId>org.apache.poi</groupId>
+      <artifactId>poi</artifactId>
+      <version>5.2.4</version>


define the property for others as well.

Added

<poi.version>5.2.4</poi.version>

anup-cloudsufi · 2023-12-11T11:50:59Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormat.java

+      isRowNull = true;
+      for (int cellIndex = 0; cellIndex < row.getLastCellNum(); cellIndex++) {
+        if (cellIndex >= fields.size()) {
+         throw new IllegalArgumentException(


Move this check outside the loop.

How can i move cellIndex logic outside loop ?
cellIndex is part of this loop 🤔

I mean instead of checking it inside the loop you can do it before the loop, something like:

Suggested change

throw new IllegalArgumentException(

if (row.getLastCellNum() > fields.size()) {

throw new IllegalArgumentException(String.format("Schema contains less fields than the number of columns in the excel file. Schema fields: %s, Excel columns: %s", fields.size(), row.getLastCellNum()));

}

anup-cloudsufi · 2023-12-11T11:57:22Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormat.java

+        }
+        Cell cell = row.getCell(cellIndex, Row.MissingCellPolicy.RETURN_BLANK_AS_NULL);
+        if (cell == null) {
+          continue;


add some comments regarding why the cell is not processed or can't we ignore this continue checks and add null in line#160

Added comment.

// Blank cells are skipped, builder will set null for the field, no processing needed.

We are also using the continue logic to track isRowNull so we can't move it inside line#160

anup-cloudsufi · 2023-12-11T12:00:49Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormatConfig.java

+    "Can be either sheet name or sheet no; for example: 'Sheet1' or '0' in case user selects 'Sheet Name' or " +
+    "'Sheet Number' as 'sheet' input respectively. Sheet number starts with 0.";
+  public static final String DESC_TERMINATE_ROW = "Specify whether processing needs to be terminated in case an" +
+    " empty row is encountered while processing excel files. Options to select are true or false.";


instead of Options to select are true or false., write The default value is false. or we can remove this also.

Updated to Default value is false.

anup-cloudsufi · 2023-12-11T12:02:24Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormatDataFormatter.java

+  /**
+   * Formats the cell value of an Excel file.
+   *
+   * @param cell the cell to format


refactor the line.

Added @param type the schema type of the cell

I see, you update the comment as follows:
{@code Cell} the cell to format

anup-cloudsufi · 2023-12-11T12:06:10Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormatDataFormatter.java

+      case ERROR:
+        return null;
+      default:
+        throw new IllegalStateException(String.format("Unexpected celltype (%s)", cellType));


Elaborate the message by specifying the list of Supported types.

This is an impossible state , only possible if xls has a new cell type added in future, this is internal representation of cell by library we are using. This is an implementation details not relevant for end user.

Even in impossible state we should be informative about the error messages. So, lets rewrite the error messages something as:
Failed to format (%s) due to unsupported cell type (%s).

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormatProvider.java

anup-cloudsufi · 2023-12-11T12:19:16Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormatProvider.java

+        for (int rowIndex = rowStart; rowIndex <= rowEnd; rowIndex++) {
+          Row row = workSheet.getRow(rowIndex);
+          if (row == null) {
+            continue;


come up with some logic to ignore the usage of continue and break down the header & data processing into separate methods for better readability.

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormatProvider.java

anup-cloudsufi · 2023-12-12T13:40:11Z

format-xls/src/main/java/io/cdap/plugin/format/xls/input/XlsInputFormat.java

+        return false;
+      }
+      // Get the next row.
+      Row row = workSheet.getRow(rowIndex);


is it guaranteed that workSheet.getRow(rowIndex) never return null?

[s] Review Squashed

psainics self-assigned this Nov 30, 2023

psainics requested review from vikasrathee-cs, anup-cloudsufi and Vipinofficial11 November 30, 2023 08:17

psainics force-pushed the xls_addition branch from 3984b8e to e6ed78f Compare December 11, 2023 04:08

vikasrathee-cs reviewed Dec 11, 2023

View reviewed changes

anup-cloudsufi reviewed Dec 11, 2023

View reviewed changes

anup-cloudsufi reviewed Dec 12, 2023

View reviewed changes

psainics force-pushed the xls_addition branch 3 times, most recently from a1f6456 to 51beae1 Compare December 21, 2023 09:35

psainics force-pushed the xls_addition branch from 51beae1 to c9b31fd Compare January 1, 2024 04:53

psainics force-pushed the xls_addition branch 2 times, most recently from 698dd7e to cd5f2a0 Compare January 22, 2024 16:14

psainics force-pushed the xls_addition branch from cd5f2a0 to 1f42fb7 Compare January 31, 2024 19:40

Added format xls

5ae5ae1

[s] Review Squashed

psainics force-pushed the xls_addition branch from 1f42fb7 to 5ae5ae1 Compare February 8, 2024 03:23

psainics merged commit 55ac600 into develop Feb 26, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xls addition #9

Xls addition #9

psainics commented Nov 30, 2023

vikasrathee-cs Dec 11, 2023

psainics Dec 11, 2023

vikasrathee-cs Dec 11, 2023

psainics Dec 11, 2023

vikasrathee-cs Dec 11, 2023

psainics Dec 11, 2023

vikasrathee-cs Dec 11, 2023

psainics Dec 11, 2023

vikasrathee-cs Dec 11, 2023

vikasrathee-cs Dec 11, 2023

psainics Dec 11, 2023

anup-cloudsufi left a comment

anup-cloudsufi Dec 11, 2023

psainics Dec 11, 2023

anup-cloudsufi Dec 11, 2023

psainics Dec 11, 2023

anup-cloudsufi Dec 11, 2023

psainics Dec 11, 2023

anup-cloudsufi Dec 12, 2023

anup-cloudsufi Dec 11, 2023

psainics Dec 11, 2023

anup-cloudsufi Dec 11, 2023

psainics Dec 11, 2023

anup-cloudsufi Dec 11, 2023

psainics Dec 11, 2023

anup-cloudsufi Dec 12, 2023

anup-cloudsufi Dec 11, 2023

psainics Dec 11, 2023

anup-cloudsufi Dec 12, 2023

anup-cloudsufi Dec 11, 2023

anup-cloudsufi Dec 12, 2023

-         throw new IllegalArgumentException(
+if (row.getLastCellNum() > fields.size()) {
+         throw new IllegalArgumentException(String.format("Schema contains less fields than the number of columns in the excel file. Schema fields: %s, Excel columns: %s", fields.size(), row.getLastCellNum()));
+}

Xls addition #9

Xls addition #9

Conversation

psainics commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anup-cloudsufi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment