Add delimiter option for reading CSV files for Feathr by ahlag · Pull Request #307 · feathr-ai/feathr

ahlag · 2022-05-30T14:26:10Z

Signed-off-by: Chang Yong Lik theeahlag@gmail.com

Description

Added delimiter options

Resolves #241

How was this patch tested?

Tested locally with sbt

sbt 'testOnly com.linkedin.feathr.offline.source.dataloader.TestCsvDataLoader'
sbt 'testOnly com.linkedin.feathr.offline.util.TestSourceUtils'
sbt 'testOnly com.linkedin.feathr.offline.source.dataloader.TestBatchDataLoader'
sbt 'testOnly com.linkedin.feathr.offline.source.dataloader.hdfs.TestFileFormat'
sbt 'testOnly com.linkedin.feathr.offline.source.dataloader.TestDataLoaderFactory'

Progress Tracker

Use the Job configurations like here to implement, i.e. add a section called spark.feathr.inputFormat.csvOptions.sep which allows end users to pass the delimiters as options
in the scala code, if you search for ss.read.format("csv").option("header", "true"), there will be a bunch of places that you need to modify. Eventually they will be using something like the csv reader here (https://spark.apache.org/docs/3.2.0/sql-data-sources-csv.html).
You can get the config in different places thru something like this: sqlContext.getConf("spark.feathr.inputFormat.csvOptions.sep", ",")
Test case
Also please help the job configuration docs (https://linkedin.github.io/feathr/how-to-guides/feathr-job-configuration.html) to make sure the options are clear to end users.

Does this PR introduce any user-facing changes?

Allows users to specify delimiters

Signed-off-by: Chang Yong Lik <theeahlag@gmail.com>

Signed-off-by: Chang Yong Lik <theeahlag@gmail.com> (cherry picked from commit bc71fad93c08f6d06e40f7e289456c6a1b4d45e0)

Signed-off-by: Chang Yong Lik <theeahlag@gmail.com>

xiaoyongzhu · 2022-07-08T16:39:21Z

@ahlag thanks for the contribution! This PR looks good to me, but I'm not sure why the test fails. Spent a bit time to investigate and feel it might be caused by the newly added tests?

ahlag · 2022-07-09T05:18:31Z

@xiaoyongzhu
Did it fail in GitHub Actions or by command line e.g. sbt 'testOnly com.linkedin.feathr.offline.util.TestSourceUtils'? I tried rerunning the following commands locally but the unit cases were mot failling.

sbt 'testOnly com.linkedin.feathr.offline.source.dataloader.TestCsvDataLoader'
sbt 'testOnly com.linkedin.feathr.offline.util.TestSourceUtils'
sbt 'testOnly com.linkedin.feathr.offline.source.dataloader.TestBatchDataLoader'
sbt 'testOnly com.linkedin.feathr.offline.source.dataloader.hdfs.TestFileFormat'
sbt 'testOnly com.linkedin.feathr.offline.source.dataloader.TestDataLoaderFactory'

xiaoyongzhu · 2022-07-09T13:42:00Z

Talked with @ahlag offline and asked him to run sbt test. Feel the issue is mostly because those failed tests were relying on loadDataFrame to read the csv files

…ure/delimiter

Signed-off-by: changyonglik <theeahlag@gmail.com>

ahlag · 2022-07-10T10:54:26Z

@xiaoyongzhu
I think I found the problem. The delimiter was not pass successfully when I tried passing the options with sqlContext.
Is there a way to set the config with SparkSession in unit tests?

TestFileFormat.scala

    val sqlContext = ss.sqlContext
    sqlContext.setConf("spark.feathr.inputFormat.csvOptions.sep", "\t")

FileFormat.scala

val csvDelimiterOption = ss.sparkContext.getConf.get("spark.feathr.inputFormat.csvOptions.sep", ",")

xiaoyongzhu · 2022-07-10T10:55:53Z

@xiaoyongzhu
I think I found the problem. The delimiter was not pass successfully when I tried passing the options with sqlContext.
Is there a way to set the config with SparkSession in unit tests?

TestFileFormat.scala
    val sqlContext = ss.sqlContext
    sqlContext.setConf("spark.feathr.inputFormat.csvOptions.sep", "\t")
FileFormat.scala
val csvDelimiterOption = ss.sparkContext.getConf.get("spark.feathr.inputFormat.csvOptions.sep", ",")

Hmm it's a bit weird. Is it possible to force set the delimiters?

ahlag · 2022-07-10T11:21:58Z

Look like there can only be one SparkContext

[info] TestFileFormat:
[info] - testLoadDataFrame
[info] - testLoadDataFrameWithCsvDelimiterOption *** FAILED ***
[info]   org.apache.spark.SparkException: Only one SparkContext should be running in this JVM (see SPARK-2243).The currently running SparkContext was created at:

ahlag · 2022-07-10T13:46:29Z

I think I will try a new approach. Since an existing SparkContext cannot be edited nor can another one be created, I will test it with e2e by passing the config from the client.

xiaoyongzhu · 2022-07-11T04:00:16Z

I think I will try a new approach. Since an existing SparkContext cannot be edited nor can another one be created, I will test it with e2e by passing the config from the client.

I did some research and found this answer:
https://stackoverflow.com/a/44613011

sqlContext.setConf("spark.sql.shuffle.partitions", "10") will set the property parameter for whole application before logicalPlan is generated.

sqlContext.sql("set spark.sql.shuffle.partitions=15") will also set the property but only for particular query and is generated at the time of logicalPlan creation.

Choosing between them depends on what your requirement is.

Maybe you can try sqlContext.sql?

ahlag · 2022-07-11T05:28:23Z

Ok, I'll give this a shot

ahlag · 2022-07-27T03:20:55Z

@xiaoyongzhu
Ok! I have updated the release version.

hangfei

Also update wiki that we support tsv.

docs/how-to-guides/feathr-job-configuration.md

src/main/scala/com/linkedin/feathr/offline/source/dataloader/BatchDataLoader.scala

src/main/scala/com/linkedin/feathr/offline/source/dataloader/hdfs/FileFormat.scala

src/test/scala/com/linkedin/feathr/offline/util/TestSourceUtils.scala

src/main/scala/com/linkedin/feathr/offline/util/SourceUtils.scala

Signed-off-by: changyonglik <theeahlag@gmail.com>

ahlag · 2022-08-04T13:44:38Z

@xiaoyongzhu @hangfei
I have finished the changes. Could you review?

blrchen · 2022-08-16T12:00:46Z

@ahlag would you mind merge latest main and resolve the conflicts so that we can get this merged, thanks for your time!

ahlag · 2022-08-16T14:22:39Z

@xiaoyongzhu @blrchen
Done! Could you merge it today? Cause I am afraid it might have new conflicts.

* Added documentation Signed-off-by: Chang Yong Lik <theeahlag@gmail.com> * Added delimiter to CSVLoader Signed-off-by: Chang Yong Lik <theeahlag@gmail.com> (cherry picked from commit bc71fad93c08f6d06e40f7e289456c6a1b4d45e0) * Added delimiter to BatchDataLoader, FileFormat and SourceUtils Signed-off-by: Chang Yong Lik <theeahlag@gmail.com> * Added test case for BatchDataLoader Signed-off-by: Chang Yong Lik <theeahlag@gmail.com> * Added test case for FileFormat Signed-off-by: Chang Yong Lik <theeahlag@gmail.com> * Added test case for BatchDataLoader Signed-off-by: Chang Yong Lik <theeahlag@gmail.com> * Added test case and fixed indent Signed-off-by: Chang Yong Lik <theeahlag@gmail.com> * Passing failure Signed-off-by: changyonglik <theeahlag@gmail.com> * Removed unused imports from BatchDataLoader Signed-off-by: changyonglik <theeahlag@gmail.com> * Fixed test failures Signed-off-by: changyonglik <theeahlag@gmail.com> * Added release version Signed-off-by: changyonglik <theeahlag@gmail.com> * Removed tailing space Signed-off-by: changyonglik <theeahlag@gmail.com> * Removed wildcard imports Signed-off-by: changyonglik <theeahlag@gmail.com> * Paraphrased comments and docstring Signed-off-by: changyonglik <theeahlag@gmail.com> * Added DelimiterUtils Signed-off-by: changyonglik <theeahlag@gmail.com> * Refactored utils Signed-off-by: changyonglik <theeahlag@gmail.com> * Updated wiki to support both tsv and csv Signed-off-by: changyonglik <theeahlag@gmail.com> * Fixed spelling error Signed-off-by: changyonglik <theeahlag@gmail.com> * trigger GitHub actions Signed-off-by: Chang Yong Lik <theeahlag@gmail.com> Signed-off-by: changyonglik <theeahlag@gmail.com>

Added documentation

36fe0b3

Signed-off-by: Chang Yong Lik <theeahlag@gmail.com>

ahlag force-pushed the feature/delimiter branch 2 times, most recently from 2b1c0a6 to ddb5643 Compare May 30, 2022 15:24

ahlag force-pushed the feature/delimiter branch 2 times, most recently from c764d1e to 36fe0b3 Compare June 11, 2022 15:56

xiaoyongzhu added the safe to test Tag to execute build pipeline for a PR from forked repo label Jun 15, 2022

ahlag force-pushed the feature/delimiter branch from ca7e91f to 3f17359 Compare June 21, 2022 11:58

Added delimiter to CSVLoader

b5c4e12

Signed-off-by: Chang Yong Lik <theeahlag@gmail.com> (cherry picked from commit bc71fad93c08f6d06e40f7e289456c6a1b4d45e0)

ahlag force-pushed the feature/delimiter branch from 3f17359 to b5c4e12 Compare June 23, 2022 06:55

ahlag and others added 4 commits June 23, 2022 16:27

Added delimiter to BatchDataLoader, FileFormat and SourceUtils

802be28

Signed-off-by: Chang Yong Lik <theeahlag@gmail.com>

Added test case for BatchDataLoader

542952d

Signed-off-by: Chang Yong Lik <theeahlag@gmail.com>

Merge branch 'main' into feature/delimiter

67f787e

Added test case for FileFormat

6229f33

Signed-off-by: Chang Yong Lik <theeahlag@gmail.com>

ahlag force-pushed the feature/delimiter branch from 86637e4 to 6229f33 Compare June 24, 2022 03:48

ahlag added 2 commits June 24, 2022 13:08

Added test case for BatchDataLoader

aa43f72

Signed-off-by: Chang Yong Lik <theeahlag@gmail.com>

Added test case and fixed indent

d3142e9

Signed-off-by: Chang Yong Lik <theeahlag@gmail.com>

ahlag changed the title ~~[WIP] Add delimiter option for reading CSV files for Feathr~~ Add delimiter option for reading CSV files for Feathr Jun 24, 2022

Merge branch 'feature/delimiter' of github.com:ahlag/feathr into feat…

d2c93cc

…ure/delimiter

ahlag force-pushed the feature/delimiter branch from 129fafa to d2c93cc Compare July 10, 2022 10:45

Passing failure

54f5543

Signed-off-by: changyonglik <theeahlag@gmail.com>

ahlag mentioned this pull request Jul 11, 2022

[Test/WIP] Test Branch for Add delimiter option for reading CSV files for Feathr #307 #451

Closed

ahlag dismissed xiaoyongzhu’s stale review via 4b4ffe9 July 27, 2022 03:20

xiaoyongzhu previously approved these changes Jul 27, 2022

View reviewed changes

hangfei reviewed Jul 31, 2022

View reviewed changes

Removed tailing space

5e28225

Signed-off-by: changyonglik <theeahlag@gmail.com>

ahlag dismissed xiaoyongzhu’s stale review via 5e28225 August 1, 2022 14:30

ahlag added 2 commits August 2, 2022 23:17

Removed wildcard imports

0ffa608

Signed-off-by: changyonglik <theeahlag@gmail.com>

Paraphrased comments and docstring

3ec3fdf

Signed-off-by: changyonglik <theeahlag@gmail.com>

ahlag force-pushed the feature/delimiter branch from 880ea90 to 3ec3fdf Compare August 2, 2022 15:06

Added DelimiterUtils

52eb35f

Signed-off-by: changyonglik <theeahlag@gmail.com>

ahlag force-pushed the feature/delimiter branch 2 times, most recently from 3842194 to 7711253 Compare August 4, 2022 13:00

Refactored utils

2d41211

Signed-off-by: changyonglik <theeahlag@gmail.com>

ahlag force-pushed the feature/delimiter branch from 062d916 to 2d41211 Compare August 4, 2022 13:13

Updated wiki to support both tsv and csv

0b01cc6

Signed-off-by: changyonglik <theeahlag@gmail.com>

ahlag force-pushed the feature/delimiter branch from f2ecf06 to 0b01cc6 Compare August 4, 2022 13:24

Fixed spelling error

b4e55fb

Signed-off-by: changyonglik <theeahlag@gmail.com>

ahlag force-pushed the feature/delimiter branch from 78b3cbd to b4e55fb Compare August 4, 2022 13:36

xiaoyongzhu previously approved these changes Aug 11, 2022

View reviewed changes

Merge branch 'main' into feature/delimiter

b7aa00e

ahlag dismissed xiaoyongzhu’s stale review via b7aa00e August 16, 2022 14:09

trigger GitHub actions

53801a3

xiaoyongzhu approved these changes Aug 16, 2022

View reviewed changes

blrchen approved these changes Aug 16, 2022

View reviewed changes

xiaoyongzhu merged commit 6a0aba3 into feathr-ai:main Aug 16, 2022

ahlag deleted the feature/delimiter branch August 17, 2022 08:05

Conversation

ahlag commented May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How was this patch tested?

Progress Tracker

Does this PR introduce any user-facing changes?

Uh oh!

xiaoyongzhu commented Jul 8, 2022

Uh oh!

ahlag commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiaoyongzhu commented Jul 9, 2022

Uh oh!

ahlag commented Jul 10, 2022

Uh oh!

xiaoyongzhu commented Jul 10, 2022

Uh oh!

ahlag commented Jul 10, 2022

Uh oh!

ahlag commented Jul 10, 2022

Uh oh!

xiaoyongzhu commented Jul 11, 2022

Uh oh!

ahlag commented Jul 11, 2022

Uh oh!

ahlag commented Jul 27, 2022

Uh oh!

hangfei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahlag commented Aug 4, 2022

Uh oh!

blrchen commented Aug 16, 2022

Uh oh!

ahlag commented Aug 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ahlag commented May 30, 2022 •

edited

Loading

ahlag commented Jul 9, 2022 •

edited

Loading