Skip to content

Conversation

@lightalloy
Copy link

  • вынесла код в service object

  • тест на основную функциональность

  • оптимизировала вставку в бд

  • оптмизировала загрузку страницы

described_class.call(Rails.root.join("fixtures/example.json"))
end.to change(City, :count).by(2).and change(Service, :count).by(2).and change(Bus, :count).by(1).and change(Trip, :count).by(10)

expect(Bus.last.services.count).to eq(2)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Базовый тест, можно бы разбить, но для наших целей хватит))

Также не помешал бы тест на производительность.


Решила рискнуть и попробовать на `1M.json`, получилось за 30 секунд. Как так? Все записи на месте в бд.

## Б. Отображение расписаний
Copy link
Author

@lightalloy lightalloy Feb 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не помешало бы реквест-тест добавить, моя недоработка )
Хоть и изменения минимальные в этом пункте.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

в ПР видел ещё тесты на view, вот это крутотень конечно

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну реквест- типа интеграционные, покроют и вью (в той мере, в к-й напишем :)

t.index ["name"], name: "index_services_on_name", unique: true
end

create_table "trips", force: :cascade do |t|
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вообще думаю, что индексы на to_id и тд связи не помешали бы ))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Но почему то нигде не показало что поиск по Trip какой то медленный запрос, я поэтому и не стал ничего добавлять. Надо следовать мантре)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Согласна. В реальном приложении важна ещё согласованность данных, это, конечно про foreign key, но индекс обычно сразу тоже.
Сейчас попробовала для интереса добавить внешние ключи и индексы. Конечно, вставка стала намного медленнее - ведь субд индексы ещё поддерживать нужно в актуальном состоянии.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А отображение страницы с индексами - первый запрос так же +-, последующие в 2 раза быстрее где-то.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Т.е. как всегда - решение зависит от целей :)

@from = City.find_by_name!(params[:from])
@to = City.find_by_name!(params[:to])
@trips = Trip.where(from: @from, to: @to).order(:start_time)
@trips = Trip.where(from: @from, to: @to).includes(bus: :services).order(:start_time)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪
Любопытно только, почему ты добавила его после where?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не специально. Вообще ar соберёт.

# вот такой insert into buses_services
# вообще чаще используем has_and_belongs_to_many , потому что часто в связке потом нужны доп. данные и таймстемпы, и свой id
# тогда можно было бы insert_all использовать
if buses_services.present?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ухты, не подумал что так можно было buses_services одним запросом вставить


Время: 632

Решила, что этого хватит.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Аналогичное решение))

Copy link

@araslanov-e araslanov-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В)

end

def call
json = JSON.parse(File.read(file))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Можно поробовать разбирать JSON потоков
https://github.com/dgraham/yajl-ffi
https://github.com/dgraham/json-stream

И ваще курутотень будет)

Copy link
Collaborator

@spajic spajic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 ✅

gem 'rspec-rails'
gem 'rack-mini-profiler'
gem 'ruby-prof'
gem 'strong_migrations'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3

@@ -0,0 +1,94 @@
class TripsImporter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

лайк за отдельный класс

<% end %>
</ul>
<%= render "delimiter" %>
====================================================
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

можно оставить паршлы, применить рендеринг коллекций и там даже можно delimiter (spacer) задать параметром https://guides.rubyonrails.org/layouts_and_rendering.html#spacer-templates

так будет не настолько тормозить, и при этом сохраняется удобство декомпозиции на паршлы

Сначала попробовала поэтапно идти:

- переписала на один insert trips, не трогая другие части; стало ещё медленнее - откатила пока
- добавляла уникальные индексы на справочные данные + использовала upsert, но во-первых не так уж сильно оптимизировало, во-вторых оказалось, что `upsert` не возвращает id, если нет вставки. Тест не отловил, т.к. там одна поездка. Дописывать тест не стала (поленилась), но в реальном приложении нужно.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

при вставке какого-то большого объёма данных может иметь смысл наоборот удалить индексы, вставить все данные, а потом уже пересоздать индексы заново

@from = City.find_by_name!(params[:from])
@to = City.find_by_name!(params[:to])
@trips = Trip.where(from: @from, to: @to).order(:start_time)
@trips = Trip.where(from: @from, to: @to).includes(bus: :services).order(:start_time)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

по идее не помешало бы ещё индекс на trips [from_id, to_id], можно составной

именно на время рендеринга это сильно не влияет (так как процент на выполнение этого SQL маленький), но если бы мы заходили со стороны оптимизации БД - там бы это очень помогло;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants