2
\$\begingroup\$

This is the first application or really any Scala I have every written. So far it functions as I would hope it would. I just found this community and would love some peer review of possible improvements or just tell me flat out what I'm doing wrong.

This code is supposed to read a table of tables then take all of the values from that table transpose the columns into rows and write them all to a master table with three additional columns. So far it does everything except the looping which I just haven't gotten to yet. This question is not about how to do the looping and really only hoping to critique what I have here so far. I did change some variable names and a few things I had to mask.

//how to call
//spark-submit --verbose --deploy-mode cluster --master yarn --class App scala-maven-0.1-SNAPSHOT.jar
import org.apache.spark.sql.{ SQLContext, SparkSession }
import org.apache.spark.sql._
import org.apache.spark.sql.types._
import org.apache.spark.sql.functions._
import java.util.Calendar

//this will call createStuff function for each table_name value in table
//TODO: Add loop and call CreateStuff with values from list returned from stuff_to_run
object App {
  def main(args: Array[String]) {
    //do I have to create the SparkSession value twice or can I pass this into the function?
    val spark = SparkSession.builder().appName("App").enableHiveSupport().getOrCreate()
    val stuff_to_run = spark.sql("select table_name from schema.table").rdd.map(r => r(0)).collect.toList
    println(stuff_to_run)
    CreateStuff("stuff");
  }

  def CreateStuff(stuffName: String): Unit = {
    var tablename = stuffName
    var column_stack = ""
    val spark = SparkSession.builder().appName("App").enableHiveSupport().getOrCreate()
    //is there a better way to get year and month
    val Year = Calendar.getInstance.get(Calendar.YEAR)
    val Month = Calendar.getInstance.get(Calendar.MONTH) + 1
    val twodigitmonth = "%02d".format(Month)
    val yrmnth = Year.toString + twodigitmonth.toString
    //get all of the data from the base table
    val raw_table = spark.sql("select *, '" + tablename + "' as stuffname, '" + yrmnth + "' as yrmnth from schema." + tablename)
    //transpose all columns into rows
    val string_table = raw_table.select(raw_table.columns.map(c => col(c).cast(StringType)): _*)
    //get column besides 3 defined these are the only ones which are not dynamic
    val selectColumns2 = string_table.columns.toSeq.filter(x => (x != "val1" && x != "stuffname" && x != "yrmnth"))
    val columnCount = selectColumns2.size
    //comma and quote
    selectColumns2.foreach { e =>
      column_stack += "'" + e + "', " + e + ", "
    }

    val collist = column_stack.mkString("").dropRight(1)
    import spark.sqlContext.implicits._

    val unPivotDF = string_table.select($"val1", $"stuffname", $"yrmnth",
      expr("stack(" + selectColumns2.size + ", " + collist.dropRight(1) + ") as (fieldname, fieldvalue)"))
    //unPivotDF.show()
    //insert records into combined table
    unPivotDF.createOrReplaceTempView("stuff_temp")
    spark.sql("insert into schema.combined_stuff select * from stuff_temp")
  }
}
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Immediately I'd ask if there's any specific style guide that allows these vastly different names, otherwise I'd suggest following IDE hints and/or a linter and rename the variables and methods to be more consistent (e.g. [cC]amelCase for everything).

Also, "stuff" is not a great name for anything ... what stuff are we talking about? In fact the first line of CreateStuff already shows that there's a better name, tableName for the parameter, probably createTable for the method. yrmnth is especially bad too, three vowels saved from yearMonth and in the process made the name unreadable for the casual observer.

Seems like the spark session is created twice and the comment even says so - I'd suggest passing the variable into the method, that's fairly straightforward by either passing it as a parameter, or extending App with a member variable.

The SQL queries are created by concatenating strings and there's usually better ways, here probably a query builder.

columnCount is unused, selectColumns2 has 2 as the suffix, even though there's no 1, that can just be selectColumns.

The loop for columnStack could be in its own method so it's a little small section that could be tested on its own too, returning what is now collist, formatColumnsList(columns: Seq[String]): String perhaps. Again, if a query builder could do all this it would be a bit cleaner than concatenating strings.

The construction for yrmnth is pretty convoluted for what it does, I'd suggest finding a more succinct approach, e.g.

val yearMonth = DateTimeFormatter.ofPattern("yyyyMM").format(LocalDateTime.now())

That would require a recent Java for java.time.LocalDateTime and java.time.format.DateTimeFormatter. But even without there should be better ways. If everything fails the whole section should at least be in its own method, e.g. formatDate(calendar Calendar): String.

The bottom part of unPivotDF uses a temporary table - is that the best way to go, or could the data be inserted directly?

\$\endgroup\$
0
\$\begingroup\$

tablename doesn't need to be a var, it's never mutated, and it appears to be totally unneeded. Use the method's passed parameter directly.

column_stack is also unneeded. You can go directly from selectColumns2 to collist without an intermediate variable or dropRight() adjustments.

val collist = selectColumns2.flatMap(e => Seq(s"'$e'",e)).mkString(", ")
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.