Add a `join` method

[zhiburt]

Add a `join` method

[zhiburt]

FYI
https://github.com/senk8/tabled/blob/add-join-method/src/lib.rs#L266-L289

Looks good 💯

Though I would change a couple of things:

• I guess we need to handle such case if self.grid.count_columns() != other.grid.count_columns(), probably by making empty cells?
• I guess there's no reason to let other = other.with(Disable::Row(..1));?
• Making this not a function but a component like other's could be cool? (a struct which implements TableOption, see all others like Disable)

I tried to join the two grids together, using Grid::get_content to copy only the data, but this way the style information of table1 and table2 is lost. In this case, how should the style be determined ?

Actually never was thinking that way.
I was thinking that we will use all styles.

If you change your inner loop to the following.
It will work as I was thinking.

let cell_settings = if row < self.grid.count_rows() {
   self.grid.get_settings(row, column)
} else {
   let row = row - self.grid.count_rows();
   other.grid.get_settings(row, column)
};

new_grid.set(&Entity::Cell(row,column),cell_settings.border_restriction(false));         

This is join in basic.rs

┌─────────┬─────────────┬───────────┬─────────┐
│  name   │  based_on   │ is_active │ is_cool │
├─────────┼─────────────┼───────────┼─────────┤
│ Manjaro │ Arch        │ true      │ true    │
├─────────┼─────────────┼───────────┼─────────┤
│ Debian  │ Independent │ true      │ true    │
├─────────┼─────────────┼───────────┼─────────┤
│ Debian  │ Independent │ true      │ true    │
└---------+-------------+-----------+---------┘
  Manjaro | Arch        | true      | true     
  Debian  | Independent | true      | true     

What do you think which is better?

[zhiburt]

Hi @senk8

You're welcome.
Feel free to contact me if there is any questions.

I haven't noticed your comment in a first place,
I hope the merge of #35 did not affect you 😥

[zhiburt]

Note1:

I can imagine the implementation could be something similar to

tabled/src/disable.rs

Lines 58 to 75 in d5aa14d

	let mut new_grid = Grid::new(new_row_size, grid.count_columns());
	for column in 0..grid.count_columns() {
	let mut new_row_index = 0;
	for row in 0..grid.count_rows() {
	let is_row_deleted = row >= x && row < y;
	if is_row_deleted {
	continue;
	}
	let cell_settings =
	grid.get_settings(row, column).border_restriction(false);
	new_grid.set(&Entity::Cell(new_row_index, column), cell_settings);
	new_row_index += 1;
	}
	}
	*grid = new_grid;

Here we delete row.
But in join case we need to add rows/columns.

And just to note that papegrid::Grid is intentionally immutable from perspective of rows/columns. We can't add any rows and columns dynamically.
We can only change data and styles.

*There are a bunch of things that must be documented 😄

[zhiburt]

Note2:

The method could be exposed via an object which implements TableOption.
This way we could set a different options.

Something like this.

table1.with(Join::inner(table2))

Not sure which is the best.
Just thoughts.

[zhiburt]

Note3:

I guess we shouldn't be worried about styles.
As we do in Dissable and Panel.

The style could be changed after join.

So essentially the main thing seems to be coping the data.

[zhiburt]

let other = other.with(Disable::Row(. .1)); because I'm trying to remove the header of other. However, I have confirmed that the header is not removed correctly this way. like this

     name       | Security | Embeded | Frontend | Unknown 
-----------------+----------+---------+----------+---------
Terri Kshlerin  |          |    +    |          |         
Catalina Dicki  |    +     |         |          |         
Jennie Schmeler |          |         |    +     |         
 Maxim Zhiburt  |          |         |          |    +    
-----------------+----------+---------+----------+---------
   Kshlerin     |          |    +    |          |         
     Dicki      |    +     |         |          |         
    Zhiburt     |          |         |          |    +    

Your concern is this -----------------+----------+---------+----------+--------- line?
Don't worry.
At least in my understanding its fine, we can't keep styles correct in any way.
Because its not clear what is correct 😄

Person then will able to call Style one more time and everything will be fixed.

let table1 = Table::new(data1).with(Style::pseudol());
let table2 = Table::new(data2).with(Style::psql());
let table3 = table1.join(table2).with(Style::Ascii);

let other = other.with(Disable::Row(. .1)); because I'm trying to remove the header of other. However, I have confirmed that the header is not removed correctly this way. like this

🤔 But do we actually need to remove header?

*Not an expert in vertical joins :)

[zhiburt]

I have a question.

let table = Table::new(people)
                          .with(Join::inner(cars));

What is the meaning of this inner? Is it different from a vertical or horizontal join?

The idea was that it is a setting of a join.
The name is not as important. (Actually a bit see below)
So essentially it could be Join::vertical(cars) and Join::horizontal().

In SQL there are different flavors of horizontal joins.
By default its called inner (I may be wrong).
There's also left join and right join.

But I am not sure if we can somehow address it here.
I'll think it though.

[zhiburt]

pub enum Join {
    Vertical(Table),
    Horizontal(Table),
}

Seems good 💯
that's what I meant ultimately.

Though I haven't tested it.

You can open a PR and we can discuss the rest there.

[senk8]

Hi, @zhiburt.

I have opened PR. Check it out 😄 .

[zhiburt]

I was looking if any library already has a similar functionality.
And I found out that pandas in python does support it.

What's interesting there, is that they actually looking for the same columns on vertical join.
They match 2 data sets columns and if there's the same column. They don't create a new one.

Your PR #40 covers only a case where columns names in the same way.

Here's an example what I mean.
You can use https://www.online-python.com/ to run it.

import pandas as pd

test_list = [['a','b','c'], ['AA','BB','CC']]

data1 = pd.DataFrame(test_list, columns=['col_A', 'col_B', 'col_C'])
data2 = pd.DataFrame(test_list, columns=['col_A', 'col_2', 'col_3'])

# vertical
air_quality = pd.concat([data1, data2], axis=0)

print(air_quality.head())

Overall I think is a good option.
But then I tend to think it would be more valuable if we have a type information at match stage.

So I am not sure if your PR must include it as I even not sure if it's valuable.

Well free to express any ideas in case you have.

[zhiburt]

But then I tend to think it would be more valuable if we have a type information at match stage.

I'm sorry, but what do you mean? Is the "match" meant to be a "match expression"?

I meant to say that when we do table1.with(join::Join::Vertical(table2)); we don't know the real type of columns (rust type).
If we would have a Table::join(data1, data2) we would have the types.
So what I essentially meant is we could join data but not tables.

But overall I still don't see a use case for it.

Just thoughts.

[senk8]

Hi @zhiburt , I'm interested in this.
Can I work on this?

[zhiburt]

Hey @senk8

Might you need some help?

[senk8]

Thank you, @zhiburt!

let table3 = table1.join(table2);

I tried to join the two grids together, using Grid::get_content to copy only the data, but this way the style information of table1 and table2 is lost. In this case, how should the style be determined ?

[senk8]

FYI
https://github.com/senk8/tabled/blob/add-join-method/src/lib.rs#L266-L289

[zhiburt]

Hhhm looking at this table I started to ask myself if join shall work like this.

I guess we need to merge columns not rows?

https://en.wikipedia.org/wiki/Join_(SQL)

Here's an example.

#[derive(Tabled)]
struct Person {
    name: String,
    age: usize,
}

#[derive(Tabled)]
struct Car {
    model: String,
    color: String,
    owner: String,
}

let people: Vec<Person> = ...
let cars: Vec<Car> = ....

let table = Table::new(people)
                          .with(Join::inner(cars));

// result
   name  │ age │ model │ color │owner │
=======================================
  senk8    *   Ferari    red    senk8

Does this make sense?

Thoughts: tuples already do such type of thing (See README.md). So for achiving an example above Vec<(Car, People)> can be created.

I think such method is still valuable in cases where len(cars) != len(people).
To mitigate complexity of client code for such a case.

So what do you think about it?

[zhiburt]

But this interface is not very versatile ....
It would be cool if we could join tables by lambda.
But we lose type information at the point.

But that's fine.
Just thoughts.

Feel free to express your ideas if you have them 😄

[senk8]

Thanks for your rapid response !

If you change your inner loop to the following.
It will work as I was thinking.
let cell_settings = if row < self.grid.count_rows() {
self.grid.get_settings(row, column)
} else {
let row = row - self.grid.count_rows();
other.grid.get_settings(row, column)
new_grid.set(&Entity::Cell(row,column),cell_settings.border_restriction(false));
};

LGTM. But, If I use that way, it looks like this. For example, let table1 and table2 be as follows:

    let data1 = vec![
        (Developer("Terri Kshlerin"), Domain::Embeded),
        (Developer("Catalina Dicki"), Domain::Security),
        (Developer("Jennie Schmeler"), Domain::Frontend),
        (Developer("Maxim Zhiburt"), Domain::Unknown),
    ];

    let table1 = Table::new(data1).with(Style::psql());

    let data2 = vec![
        (Developer("Kshlerin"), Domain::Embeded),
        (Developer("Dicki"), Domain::Security),
        (Developer("Zhiburt"), Domain::Unknown),
        (Developer("Schmeler"), Domain::Frontend),
    ];

    let table2 = Table::new(data2).with(Style::psql());
    let table3 = table1.join(table2);

table3 looks like the following.

      name       | Security | Embeded | Frontend | Unknown 
-----------------+----------+---------+----------+---------
 Terri Kshlerin  |          |    +    |          |         
 Catalina Dicki  |    +     |         |          |         
 Jennie Schmeler |          |         |    +     |         
  Maxim Zhiburt  |          |         |          |    +    
      name       | Security | Embeded | Frontend | Unknown 
-----------------+----------+---------+----------+---------
    Kshlerin     |          |    +    |          |         
      Dicki      |    +     |         |          |         
     Zhiburt     |          |         |          |    +    

I wish there was a way to just remove the header. Do you know how to do this ?

let other = other.with(Disable::Row(. .1)); because I'm trying to remove the header of other. However, I have confirmed that the header is not removed correctly this way. like this

      name       | Security | Embeded | Frontend | Unknown 
-----------------+----------+---------+----------+---------
 Terri Kshlerin  |          |    +    |          |         
 Catalina Dicki  |    +     |         |          |         
 Jennie Schmeler |          |         |    +     |         
  Maxim Zhiburt  |          |         |          |    +    
-----------------+----------+---------+----------+---------
    Kshlerin     |          |    +    |          |         
      Dicki      |    +     |         |          |         
     Zhiburt     |          |         |          |    +    

P.S.

I think it would be better to merge the headers without removing them, so that the user can get the Intuitive behavior. Don't worry about it.

Making this not a function but a component like other's could be cool? (a struct which implements TableOption, see all
others like Disable)

I think so ! I'm working on this.

[senk8]

@zhiburt

I guess we need to handle such case if self.grid.count_columns() != other.grid.count_columns(), probably by making
empty cells?

Hhhm looking at this table I started to ask myself if join shall work like this.

I guess we need to merge columns not rows?

https://en.wikipedia.org/wiki/Join_(SQL)

Here's an example.

Oh! I was only thinking about the following vertical join case.

      name       | Security | Embeded | Frontend | Unknown 
-----------------+----------+---------+----------+---------
 Terri Kshlerin  |          |    +    |          |         
 Catalina Dicki  |    +     |         |          |         
 Jennie Schmeler |          |         |    +     |         
  Maxim Zhiburt  |          |         |          |    +    
-----------------+----------+---------+----------+---------
    Kshlerin     |          |    +    |          |         
      Dicki      |    +     |         |          |         
     Zhiburt     |          |         |          |    +    

I'll make it possible to join horizontally.

[senk8]

@zhiburt
Sorry for the late reply 😔. I implements "join" with TableOption as follows .
senk8@81c6a6c

I have a question.

let table = Table::new(people)
                          .with(Join::inner(cars));

What is the meaning of this inner? Is it different from a vertical or horizontal join?

[senk8]

But then I tend to think it would be more valuable if we have a type information at match stage.

I'm sorry, but what do you mean? Is the "match" meant to be a "match expression"?

I think this is a very good idea 👍 . For some users, it may not be intuitive to have columns with the same name. There will be more options, and we will need to consider the design 🤔 .

[zhiburt]

The logic was discussed here was closed in #40

I started to recognize that current Join is not particularly join.
So it was renamed to Concat which more ergonomic name for this functionality.

The join logic its self must be determined yet.
As it turns out not to be as straightforward.
I'll open a new issue for this when the requirements will be more clear.